fix(IRRemoteControlDevice): raise RuntimeError on undetected control_type in send_command()#698
Conversation
…tected control_type
Some IR devices (e.g. Aubess IR PRO, product ID wnykq) return an empty
dps {} from status() instead of advertising their supported DPs. The
previous detection logic would silently leave control_type=0, and
send_command() would then silently no-op — making it very hard to
diagnose why no IR signals were being sent.
Two fixes:
1. detect_control_type(): When status() returns an empty dps dict,
fall back to control_type=1 (DPS 201) rather than giving up.
DPS 201 is the original/most common IR control DP and is a safe
default for devices that don't advertise their DPs. Also change
the detection failure path from 'leave at 0' to 'default to 1'
with an updated warning message.
2. send_command(): Raise a clear RuntimeError when control_type is
still 0 instead of silently dropping the command. This surfaces
the problem immediately with an actionable message.
Fixes issue jasonacox#492 (Aubess WiFi IR Controller S16 - cannot control device
via IRRemoteControlDevice Contrib class when dps {} is empty).
There was a problem hiding this comment.
Pull request overview
Improves IRRemoteControlDevice robustness for IR controllers that return an empty dps {} from status(), avoiding silent no-op behavior and making failures more diagnosable for users (e.g., Aubess IR PRO / issue #492).
Changes:
- Add an empty-
dpsfallback indetect_control_type()to default tocontrol_type=1(DPS 201). - Default to
control_type=1when detection fails, instead of leavingcontrol_type=0. - Raise a clear
RuntimeErrorinsend_command()ifcontrol_typeis still0.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.set_socketTimeout( old_timeout ) | ||
| self.set_socketPersistent( old_persist ) | ||
|
|
||
| def send_command( self, mode, data={} ): |
There was a problem hiding this comment.
send_command() uses a mutable default argument (data={}), which can lead to shared state across calls if the dict is ever mutated (directly or indirectly). Prefer data=None and assign {} inside the function to avoid this class of bugs.
| def send_command( self, mode, data={} ): | |
| def send_command( self, mode, data=None ): | |
| if data is None: | |
| data = {} |
There was a problem hiding this comment.
Good catch — fixed in the latest commit (bf1c0b8). Changed data={} to data=None with an explicit if data is None: data = {} guard inside the function. Agreed this is the right pattern.
| log.warning( 'Detect control type failed! control_type= must be set manually' ) | ||
| log.warning( 'Detect control type failed! Defaulting to control_type=1 (DPS 201). ' | ||
| 'Override with control_type=1 or control_type=2 if needed.' ) | ||
| self.control_type = 1 |
There was a problem hiding this comment.
New control-type detection behavior (empty-dps fallback and default-to-1 on detection failure) is not covered by unit tests. Since the repo already has unittest coverage for RFRemoteControlDevice in tests.py, please add similar mocked tests for IRRemoteControlDevice.detect_control_type() to lock in the empty-dps path and the failure-default path.
There was a problem hiding this comment.
Agreed that tests for the new detect_control_type() paths would be valuable. That said, I'd prefer to keep this PR focused on the robustness fix itself — the existing test suite doesn't have IRRemoteControlDevice coverage at all, so adding it properly is a meaningful effort that deserves its own PR. I'll open a follow-up specifically for IR test coverage.
| if not self.control_type: | ||
| raise RuntimeError( | ||
| 'IRRemoteControlDevice: control_type has not been detected. ' | ||
| 'Pass control_type=1 (DPS 201/202) or control_type=2 (DPS 1-13) ' | ||
| 'to the constructor, or call detect_control_type() first.' | ||
| ) |
There was a problem hiding this comment.
The new RuntimeError path in send_command() should be exercised by a unit test (e.g., force control_type=0 and assert the exception/message) so this behavior doesn’t regress back to a silent no-op.
There was a problem hiding this comment.
Same as above — the RuntimeError path in send_command() is worth locking in with a test, but I'd rather not expand this PR's scope. Will include it in the IR test coverage follow-up PR.
|
@ClusterM — since you created the @uzlonewolf @jasonacox — if this looks good to you both, it should be ready to merge. Will plan to merge in ~1 week if no objections. |
|
I disagree with blindly setting control_type to 1, this is just going to cause weird problems later. The only part of this PR we should keep is the I see no evidence that the detection failed in #492 - his problem was that the leading '0' was not removed from key1 when switching from rawdogging the JSON to using the Contrib module. |
|
@uzlonewolf — thanks for the quick review. Let me explain the reasoning behind the empty-DPS fallback before we decide what to do with it. The original issue in #492 showed a device ( My reasoning for defaulting to That said, your point has merit: "best-effort guess" can mask real detection failures and cause confusing behavior for devices where the guess is wrong. A failed detection that defaults silently to type 1 is arguably worse than a failed detection that fails loudly — at least the loud failure tells the user something is actually wrong. Where I think we agree: the So the question is really: should we keep the empty-DPS fallback alongside the RuntimeError, or drop it and let the RuntimeError do all the work? I can see the argument for dropping it — if detection genuinely fails, the RuntimeError will catch it, and the user gets a clear message to set Happy to revise the PR to remove the empty-DPS fallback if that's the direction you prefer. What do you think? |
|
@jasonacox-sam Yes, that's right. |
…meError on undetected control_type
|
@uzlonewolf — updated in |
|
Agree. @jasonacox-sam I recommend you update the PR Summary and Changes note to correctly reflect the revised change before merge. |
|
@jasonacox — PR title and description updated to reflect the revised change. Ready for merge when you are. |
|
Thanks @jasonacox-sam - good contribution. And thanks @uzlonewolf 🙏 Merging. |
Summary
send_command()would silently no-op ifdetect_control_type()failed to identify the device type — leavingcontrol_type=0with no error, no log output, and no IR signals sent. This made the failure very difficult to diagnose.Surfaced in issue #492 where a user spent considerable time debugging before finding a workaround. (Note: the root cause in #492 was a leading
0inkey1, not a detection failure — but the silent no-op behavior is worth fixing regardless.)Changes
detect_control_type()— cleaner failure loggingRemoved the empty-DPS fallback and the detection-failure default that silently set
control_type=1. If detection fails,control_typestays0and a warning is logged. Users can setcontrol_typemanually.send_command()— raise on undetected control_typeIf
send_command()is called withcontrol_type=0, raise a clearRuntimeErrorwith an actionable message instead of silently dropping the command. This surfaces the failure immediately and tells the user exactly what to do.send_command()— fix mutable default argumentChanged
data={}default argument todata=Nonewith an explicit guard inside the function, avoiding the Python mutable default argument pitfall.Review Notes
Per @uzlonewolf: blindly defaulting
control_type=1on empty DPS would mask real detection failures. TheRuntimeErrorinsend_command()is the right place to catch undetected control types.Related Issues
Related to #492