Allow discovering Broadlink device at a specific IP#13
Conversation
Reviewer's Guide by SourceryThis pull request adds functionality to discover a Broadlink device at a specific IP address. It modifies the Sequence DiagramsequenceDiagram
participant C as Client
participant A as API
participant B as Blaster DB
participant D as Broadlink Device
C->>A: GET /discoverblasters?at_ip=<value>
A->>B: get_new_blasters(at_ip=<value>)
alt at_ip provided
B->>D: broadlink.hello(at_ip)
D-->>B: Device info
else at_ip not provided
B->>D: discover_blasters()
D-->>B: List of devices
end
B-->>A: Discovered device(s)
A-->>C: JSON response with discovered device(s)
File-Level Changes
Tips and commands
|
There was a problem hiding this comment.
Hey @cdzombak - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
|
||
|
|
||
| def get_new_blasters(timeout=DISCOVERY_TIMEOUT): | ||
| def get_new_blasters(at_ip=None, timeout=DISCOVERY_TIMEOUT): |
There was a problem hiding this comment.
suggestion: Consider adding error handling for broadlink.hello(at_ip) call
The function should handle potential exceptions or errors that may occur when calling broadlink.hello(at_ip). This will improve the robustness of the code.
def get_new_blasters(at_ip=None, timeout=DISCOVERY_TIMEOUT):
try:
cnt = 0
discovered = []
if at_ip:
broadlink.hello(at_ip)
except Exception as e:
logging.error(f"Error discovering blasters: {e}")
return []
|
|
||
| def get_new_blasters(timeout=DISCOVERY_TIMEOUT): | ||
| def get_new_blasters(at_ip=None, timeout=DISCOVERY_TIMEOUT): | ||
| cnt = 0 |
There was a problem hiding this comment.
suggestion: Remove unused variable 'cnt'
The 'cnt' variable is initialized but never used. Consider removing it to improve code clarity.
| cnt = 0 | |
| def get_new_blasters(at_ip=None, timeout=DISCOVERY_TIMEOUT): | |
| discovered = [] |
| class DiscoverRESTResource(object): | ||
| def on_get(self, req, resp): | ||
| resp.body = json.dumps(blaster_db.get_new_blasters()) | ||
| resp.body = json.dumps(blaster_db.get_new_blasters(at_ip=req.get_param("at_ip", required=False))) |
There was a problem hiding this comment.
🚨 suggestion (security): Consider adding input validation for the 'at_ip' parameter
While the 'at_ip' parameter is optional, it's important to validate it as a valid IP address when provided. This will enhance the security and reliability of the API.
from ipaddress import ip_address
def validate_ip(ip):
try:
ip_address(ip)
return True
except ValueError:
return False
at_ip = req.get_param("at_ip", required=False)
if at_ip and not validate_ip(at_ip):
raise ValueError("Invalid IP address provided")
resp.body = json.dumps(blaster_db.get_new_blasters(at_ip=at_ip))
| 2. The database files are in SQLite3 format and can be hand edited if needed. I use [SQLiteStudio](https://sqlitestudio.pl/index.rvt). | ||
| 3. To reset all settings/data, simply stop the container/app, delete the two .db files, and restart. | ||
| 4. This was tested on an RM3 Mini but should theoretically support any RM device that [python-broadlink](https://github.com/mjg59/python-broadlink) does. | ||
| 5. Blasters must be unlocked (accessible via WLAN) to be controlled via this program, otherwise you may get `Authentication failed` errors (see [mjg59/python-broadlink#719](https://github.com/mjg59/python-broadlink/issues/719)). To unlock a blaster in the Broadlink app: select the device, tap "..." in the top tap, click "Property", then find the "Lock device" setting and turn it off. |
There was a problem hiding this comment.
suggestion (documentation): Fix typo: 'top tap' should be 'top right'
| 5. Blasters must be unlocked (accessible via WLAN) to be controlled via this program, otherwise you may get `Authentication failed` errors (see [mjg59/python-broadlink#719](https://github.com/mjg59/python-broadlink/issues/719)). To unlock a blaster in the Broadlink app: select the device, tap "..." in the top tap, click "Property", then find the "Lock device" setting and turn it off. | |
| 5. Blasters must be unlocked (accessible via WLAN) to be controlled via this program, otherwise you may get `Authentication failed` errors (see [mjg59/python-broadlink#719](https://github.com/mjg59/python-broadlink/issues/719)). To unlock a blaster in the Broadlink app: select the device, tap "..." in the top right, click "Property", then find the "Lock device" setting and turn it off. |
Summary by Sourcery
Introduce functionality to discover Broadlink RM blasters at a specific IP address, enhancing the discovery process for locked or otherwise undiscoverable devices. Update the documentation to reflect these changes and provide additional user guidance.
New Features:
/discoverblasters?at_ip=<value>endpoint.Enhancements:
get_new_blastersfunction to accept an optionalat_ipparameter for targeted blaster discovery.Documentation:
/discoverblasters?at_ip=<value>endpoint and provide additional guidance on unlocking blasters for control.