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

Support for the default ramp rate setting for Dimmer and KeypadLinc #235

Merged
merged 8 commits into from
Dec 14, 2020

Conversation

jordanrounds
Copy link
Contributor

This adds support for setting the ramp rate using the set_flags command. It supports a value between 0 and 31. I've tested locally on real hardware and setting the rate works without any issue. The changes I've made are basically a duplicate of the set_on_level method.

The keypad and dimmer support a default ramp rate to be set.
@tstabrawa
Copy link
Contributor

Closes #207

jordanrounds and others added 3 commits December 1, 2020 21:20
Co-authored-by: tstabrawa <59430211+tstabrawa@users.noreply.github.com>
Co-authored-by: tstabrawa <59430211+tstabrawa@users.noreply.github.com>
Co-authored-by: tstabrawa <59430211+tstabrawa@users.noreply.github.com>
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 note that this will only work with i2 or i2cs devices. There is a way to do this for i1 devices, but I am starting to question whether we want to continue adding features for such devices. They are very old now and often cause a lot of issues. It is hard to test the code as most developers do not have these devices.

I would be content to start a type of "deprecation" of i1 devices. We have the basic functionality for supporting them, I am not suggesting removing that. But I would be fine with not adding support for additional features unless an owner of i1 devices wants to step forward and write the code.

I had one comment about checking to see if the KPL is a dimmer. If we address that, then I see no reason not to merge this.

to 9 minutes.

Args:
rate (int): The default ramp rate in the range [0,31]
Copy link
Contributor

@tstabrawa tstabrawa Dec 4, 2020

Choose a reason for hiding this comment

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

Sorry for the late comment. I noticed that this code is expecting a ramp rate value that would be handed directly to the device -- so the user would be expected to know what value to use based on how those values are interpreted by devices (such as by consulting this table.)

I just wanted to raise the question - would it be better if the ramp rate were specified in seconds, and translated using the Dimmer.ramp_pretty lookup table, similar to as done in link_data_from_pretty()? (Same question applies to the changes to the Dimmer class.)

This might be a question for @krkeegan or @TD22057 to answer, though.

@krkeegan
Copy link
Collaborator

krkeegan commented Dec 4, 2020

Ahh, yes good catch, that is a better way to handle things. The code is already available. Look inside Devices/Dimmer you should see a dict of ramp_pretty. Something like the following code should handle the conversion for you:

data_3 = 0x1f
for ramp_key, ramp_value in self.ramp_pretty.items():
    if rate >= ramp_value:
        data_3 = ramp_key
        break

Then set your D3 value using data_3. That code will select the exact ramp rate if it exists or the next lowest valid value.

The rate is now passed in as float in seconds ranging from 0.1 to 540
@jordanrounds
Copy link
Contributor Author

Made the changes you suggested. Needed to add to the util to support float. Not exactly sure I handled everything necessary or am following the patterns elsewhere in the code, but that seemed like where I was supposed to be doing it.

@krkeegan krkeegan merged commit 30ec0f7 into TD22057:master Dec 14, 2020
@krkeegan
Copy link
Collaborator

Bah, this was merged into master. That was my fault, I should have looked more carefully. I am quickly resetting master before anyone else syncs to it.

I will rebase it off of dev and resubmit it for you Jordan. In the future, please create your branch off of dev.

@jordanrounds
Copy link
Contributor Author

Sorry about that. This is my first time contributing anything. I'll create the branch off of dev next time.

This was referenced Dec 14, 2020
@krkeegan
Copy link
Collaborator

No problem, it happens a lot, I should have caught it. All fixed by #253 now.

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.

3 participants