Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resume dim for non i2cs devices #423

Merged
merged 5 commits into from
Jun 8, 2021
Merged

Conversation

lnr0626
Copy link
Contributor

@lnr0626 lnr0626 commented May 25, 2021

Proposed change

Enables the resume_dim flag for non i2cs devices

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.
  • Code documentation was added where necessary

I only have i2cs devices, so will need help verifying this works properly for non i2cs devices :-)

@lnr0626 lnr0626 changed the title Resume dim non i2cs Resume dim for non i2cs devices May 25, 2021
@krkeegan
Copy link
Collaborator

Awesome, I appreciate you making that a separate method so that it can be reused for other flags.

@krkeegan
Copy link
Collaborator

My only hesitation here is the addition of the get-engine request to the refresh request. I see it is limited to only occur in cases where the engine is unknown or refresh is called with force.

I understand the benefit of this, making things "just work" for users. The concern that I have always had, is as we chain more commands to refresh we run a greater risk of complications.

I think this is probably a good addition, but I want to think about it for a minute.

@lnr0626
Copy link
Contributor Author

lnr0626 commented May 25, 2021

that makes sense - I added it mainly because i had a few devices that didn't have that set initially, and without that the devices likely wouldn't respond correctly.

An alternative would be to add some additional handling so that if we don't know the engine and we received a NACK (invalid checksum) in response to a set_operating_flags message, we either update the engine to i2cs and retry; or run get_engine and then retry

my main hesitation with just setting the engine to i2cs is that's a fairly large assumption that may break at some point.

that might also have the potential for infinite loops if (for some reason), we are never able to set the engine?

@krkeegan krkeegan changed the base branch from master to dev June 7, 2021 20:56
Copy link
Collaborator

@krkeegan krkeegan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested out refresh with the get_engine command on a few i2cs devices and some old i1 devices and it all works fine. We already do a model request, so it likely makes sense to get the engine version too.

A few small additions, but I am otherwise happy to merge this.

insteon_mqtt/device/base/Base.py Outdated Show resolved Hide resolved
tests/device/test_DimmerDev.py Show resolved Hide resolved
lnr0626 and others added 2 commits June 7, 2021 19:32
Co-authored-by: Kevin Robert Keegan <[email protected]>
Co-authored-by: Kevin Robert Keegan <[email protected]>
@lnr0626
Copy link
Contributor Author

lnr0626 commented Jun 8, 2021

those changed looked good to me, so just went ahead and commited them. thanks for the reviews and the help with testing this stuff out!

@krkeegan krkeegan merged commit f4d7f9d into TD22057:dev Jun 8, 2021
@lnr0626 lnr0626 deleted the resume-dim-non-i2cs branch July 4, 2021 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Resume Dim Feature on non-I2CS Devices
2 participants