Add Dynamic IP Update Logic for openHASP Plate#160
Add Dynamic IP Update Logic for openHASP Plate#160wjnelson78 wants to merge 2 commits intoHASwitchPlate:mainfrom
Conversation
…r_job In `__init__.py`, direct open() calls were being made on the Home Assistant event loop, leading to “Detected blocking call to open()” warnings and potential performance issues. To address this: - Moved the reading of `pages_schema.json` out of the constructor and into `async_added_to_hass`, wrapping the file read with `hass.async_add_executor_job()`. - Introduced small synchronous helper functions (`_sync_read_json`, `_sync_read_text`, etc.) for file I/O. These are called inside `async_add_executor_job()`, preventing the main loop from blocking. - Updated `async_load_page` to use the same pattern for JSON or text lines, validating JSON data against the pre-loaded schema. - Ensures compliance with Home Assistant’s async guidelines and avoids blocking the event loop with disk I/O. - Improves overall responsiveness and prevents related warnings in the HA logs.
dgomes
left a comment
dgomes
left a comment
There was a problem hiding this comment.
lets leave .gitignore alone ;)
| # Define minimal schemas so that each service is recognized as an entity service | ||
| WAKEUP_SCHEMA = cv.make_entity_service_schema({}) | ||
| PAGE_NEXT_SCHEMA = cv.make_entity_service_schema({}) | ||
| PAGE_PREV_SCHEMA = cv.make_entity_service_schema({}) |
There was a problem hiding this comment.
this is already in a different PR
| # If "ip" is reported, update config entry + device registry | ||
| if "ip" in message: | ||
| self._statusupdate["ip"] = message["ip"] | ||
| new_url = f"http://{message['ip']}/" |
There was a problem hiding this comment.
we should use a function to create the url and not write the url template in 2 different locations
| if new_url != old_url: | ||
| _LOGGER.debug("IP changed to %s, updating config entry URL...", new_url) | ||
| new_data = dict(self._entry.data) | ||
| new_data[DISCOVERED_URL] = new_url |
There was a problem hiding this comment.
why not edit self._entry.data directly ?
|
Hi @dgomes, apologies for the delayed response! I had applied the fix to my local codebase and didn't notice the review feedback until recently. I've addressed your feedback in a new, cleaner branch:
The updated changes are minimal and focused solely on the dynamic IP update feature. Would you prefer I close this PR and open a new one from the Thanks for your patience! |
|
This branch has conflicts that must be resolved, please review and dix this PR. |
There was a problem hiding this comment.
Pull request overview
This PR's title and description claim to add dynamic IP update logic for openHASP plates, but the actual code changes do NOT implement this feature. The PR description mentions checking for "ip" in MQTT payload, calling async_update_entry, and async_update_device, but none of this logic is present in the diff.
Instead, this PR primarily contains code refactoring and cleanup work:
- Code formatting and comment improvements
- Migration from Script to async_call_from_config for event handling
- Removal of the http_proxy parameter from push_image service
- Refactored file I/O operations to use dedicated helper functions
- Simplified hash generation for image IDs
Comments suppressed due to low confidence (1)
custom_components/openhasp/init.py:159
- The removal of ATTR_PROXY (http_proxy) parameter from PUSH_IMAGE_SCHEMA is a breaking API change. Users who were passing the http_proxy parameter to the push_image service will now get schema validation errors. Consider deprecating this parameter first with a warning, or document this as a breaking change in release notes.
PUSH_IMAGE_SCHEMA = vol.Schema(
{
vol.Required(ATTR_IMAGE): vol.Any(cv.url, cv.isfile),
vol.Required(ATTR_OBJECT): hasp_object,
vol.Optional(ATTR_WIDTH): cv.positive_int,
vol.Optional(ATTR_HEIGHT): cv.positive_int,
vol.Optional(ATTR_FORCE_FITSCREEN): cv.boolean,
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for service in self.event_services[event]: | ||
| await async_call_from_config( | ||
| self.hass, service, validate_config=False, variables=message | ||
| ) |
There was a problem hiding this comment.
The change from Script to async_call_from_config changes the event handling behavior. The Script class provided run context management and proper variable scoping. With async_call_from_config, there's no explicit context passed (the Context import was removed), which may affect service call tracing and automation debugging in Home Assistant. Additionally, validate_config=False bypasses safety checks that could catch configuration errors early.
| for domain in PLATFORMS: | ||
| hass.async_create_task(hass.config_entries.async_forward_entry_setup(entry, domain)) |
There was a problem hiding this comment.
Using async_forward_entry_setup in a loop with async_create_task can lead to race conditions and is deprecated. The modern Home Assistant best practice is to use async_forward_entry_setups which properly handles multiple platforms concurrently and awaits all setups to complete before returning.
| for domain in PLATFORMS: | |
| hass.async_create_task(hass.config_entries.async_forward_entry_setup(entry, domain)) | |
| await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS) |
| ).hexdigest() | ||
| async def async_push_image(self, image, obj, width=None, height=None, fitscreen=False): | ||
| """Push an image to the plate object.""" | ||
| image_id = hashlib.md5(image.encode("utf-8")).hexdigest() |
There was a problem hiding this comment.
The image_id hash calculation has been weakened. Previously it included both the image URL and the plate name in the hash (making each plate have unique image IDs), but now it only hashes the image URL. This means if multiple plates use the same image URL, they will share the same cached image in hass.data[DOMAIN][DATA_IMAGES], which could cause issues if plates have different display resolutions or if the image gets removed from the cache while still in use by another plate.
| image_id = hashlib.md5(image.encode("utf-8")).hexdigest() | |
| image_key = f"{self._topic}|{image}" | |
| image_id = hashlib.md5(image_key.encode("utf-8")).hexdigest() |
| _LOGGER.error( | ||
| "Can't change to %s, available pages are 1 to %s", page, num_pages | ||
| ) | ||
| if page <= 0 or page > num_pages: |
There was a problem hiding this comment.
Missing validation of page parameter type. The removed type checks ensured page was an integer and num_pages was an integer before performing comparisons. Now if page is passed as a string or other type, the comparison 'page <= 0 or page > num_pages' may not work correctly or could raise a TypeError at runtime.
Pull Request Description:
This PR enables the openHASP integration to detect and update the device’s IP address whenever the plate reports a new one in its statusupdate message. Specifically:
• If found, stores the new IP in Home Assistant’s config entry data (async_update_entry)
• Also updates the plate’s device registry entry (async_update_device), so the device “Link” in HA points to the current IP address
• Ensures the plate entity, device registry link, and UI references are always in sync with the actual device IP
These changes improve reliability in environments where the plate’s IP might change over time (e.g., DHCP lease expirations, new IP assignments), eliminating the need for manual URL updates.