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

Add Support for "Fast On" & "Fast Off" #478

Closed
wants to merge 8 commits into from

Conversation

DoumP
Copy link
Contributor

@DoumP DoumP commented Dec 7, 2014

Allow the state_now command to recognise fast on and fast off command.

@hollie
Copy link
Owner

hollie commented Dec 8, 2014

Looks good to me, but I don't have Insteon hardware to test on. Can other testers verify this change please?

Regards,
Lieven.

@krkeegan
Copy link
Collaborator

The code is likely fine, although it adds the on/off_fast commands to nearly every device, so we should keep an eye out for obscure devices acting strangely.

My concern is that this goes to the issues raised in #438 without really solving them. It would allow a Relay device to appear as "on_fast" even though the Relay device has no ramp rate capabilities on the Load. We are likely to end up back here again, when the next user realizes that a remotelinc can be used as a dimmer controller, but does not itself report anything other than on/off.

@DoumP
Copy link
Contributor Author

DoumP commented Dec 30, 2014

Right now some on_fast/off_fast command are sent in the insteon protocol and misterhouse is changing that to on/off. With my fix at least we get the actual insteon command in misterhouse. I'm not sure I understand everything from the #438 request but it seems about tracking the effect of the command sent on other devices, which sounds like a different issue to me. I suspect that getting the real insteon command should help but I'm not mastering misterhouse so I can't argue on the effect of the change. The only thing I can assess is that it's working fine for me so far.
I could add a flag to see if the on_fast is allow for a specific device but I don't see the point since we already received that signal from insteon so why should we check if it's allowed or not? If it's not supported on a specific device it wont come in anyway.

@krkeegan
Copy link
Collaborator

Let me try and explain.

This commit adds on_fast and off_fast states to nearly every Insteon device, so we are not just talking about the RemoteLinc. However, it does not add the dimming (1-99%) commands to the RemoteLinc.

A SwitchLinc Relay (on/off only variety) has a single "state" in MH. But in reality, the device has 2 distinct states, the Light-State and the Switch-State. The Light-State, representing the light controlled by the device, can only ever be ON or OFF. The Switch-State, representing the command most recently received from the device, can be ON, ON_FAST, 1-99%, OFF, or OFF_FAST.

Historically, the state in MH has always been strictly tied to the Light-State. #438 discusses various options for how to incorporate the Switch-State.

Since the RemoteLinc has no Light-State, the short term solution may be to classify it as a Dimmable_Device, this would add the FAST states and DIM states to the RemoteLinc only.

krkeegan added a commit to krkeegan/misterhouse that referenced this pull request Dec 30, 2014
@krkeegan
Copy link
Collaborator

I have attached a commit that matches my comments.

@DoumP
Copy link
Contributor Author

DoumP commented Dec 30, 2014

Thanks for the clarification, now I understand much better.
Thanks for the fix, I will test with that.

@DoumP
Copy link
Contributor Author

DoumP commented Jan 4, 2015

For some reason your fix doesn't work for me, I'm still getting only on/off.
I'm far from mastering perl and so far I can't figure out why it's not working.
I can use the on_fast with a switchlinc so there's definitely a way to get that working with the remotelinc.

@marcmerlin
Copy link
Collaborator

@DoumP: What's the current status? Are you waiting for @krkeegan to review your new patch which is now fully working for you?

@DoumP
Copy link
Contributor Author

DoumP commented Apr 1, 2015

Yes is does work for me but I had to copy part of the code from the Dimmable_Device which doesn't seem optimal.

@gduprey
Copy link

gduprey commented Aug 28, 2015

Howdy,

So reading over the patch/code, I get the impression that this just "smerges" the on & on_fast and off & off_fast into on or off state. Is there a way for user code to determine that an "on" was just an on vs an on_fast?

i.e. if ($myLight->state_now eq "on_fast") { do something on a double click }

Gerry

@gac410
Copy link
Contributor

gac410 commented Aug 28, 2015

I had an alternate implementation that I did ages ago, but it didn't
seem to generate any interest, due to the conflict between the light and
the switch state.

gac410@8148328

Rather than try to keep refitting it, I have stopped pulling, and I'm
stuck on the older code. It works for me, and tbh I'd rather have the
double-click controls for things like overriding timers, etc., than the
current restrictions.

So to the powers that be. Please make sure that we can get and process
the *_fast commands from devices like switchlinc-relay. I'd rather not
maintain local patches.

Thanks,
George

On 08/28/2015 04:07 PM, gduprey wrote:

Howdy,

So reading over the patch/code, I get the impression that this just
"smerges" the on & on_fast and off & off_fast into on or off state. Is
there a way for user code to determine that an "on" was just an on vs
an on_fast?

i.e. if ($myLight->state_now eq "on_fast") { do something on a double
click }

Gerry


Reply to this email directly or view it on GitHub
#478 (comment).

@DoumP
Copy link
Contributor Author

DoumP commented Aug 29, 2015

@gduprey : I've done this patch exactly to be able to use code as in your example.

@gduprey
Copy link

gduprey commented Aug 30, 2015

Sorry to keep dragging this on. I'm up for applying the patch, but how do you write code to take advantage of a double tap/on_fast command?

Right now, I have ,pl code like if ($mylight->state_now eq 'on').

With Georges patch, would I just use if ($mylight->state_now eq 'on_fast') ?

Thanks!

Gerry

@gac410
Copy link
Contributor

gac410 commented Aug 30, 2015

I do things like:

Konsole output
if ( $state = ( state_now $l_bath_fan_lt ) ) {
if ( $state eq 'on_fast' ) {
set $bath_fan_timer (0); #cancel the timer
..

and

Konsole output
if ( $state = ( state_now $l_kitchen_oh_s1 ) ) {
if ( $state eq 'off_fast' ) {
set $undercounter OFF;
set $back_hall OFF;
set $kitchen_recessed OFF;
...

George

On 08/30/2015 11:13 AM, Gerry wrote:

Sorry to keep dragging this on. I'm up for applying the patch, but how
do you write code to take advantage of a double tap/on_fast command?

Right now, I have ,pl code like if ($mylight->state_now eq 'on').

With Georges patch, would I just use if ($mylight->state_now eq
'on_fast') ?

Thanks!

Gerry


Reply to this email directly or view it on GitHub
#478 (comment).

@gduprey
Copy link

gduprey commented Aug 30, 2015

I looked int he code base and it appeared that there is already some logic for on_fast and off_fast. The above patch seemed to only be to enable that for the RELAY switch linc device too. Is that correct (i.e. all other non-RELAY devices already support on/off_fast?)

Sorry -- trying to merge several different issues, threads and messages I'm finding on this topic and all seem slightly different.

Gerry

@DoumP
Copy link
Contributor Author

DoumP commented Nov 16, 2015

I finally got it to work as krkeegan suggested.

@DoumP
Copy link
Contributor Author

DoumP commented Nov 16, 2015

Yes Gerry, my goal was to get on_fast on the remotelinc.

@marcmerlin
Copy link
Collaborator

DoumP, so if I read it right, you were able to use this existing FastOn/FastOff code and his patch is not needed anymore?
If so, do you mind closing this request?

@gac410
Copy link
Contributor

gac410 commented Jan 13, 2016

My patch referenced above - gac410@8148328 adds *_fast command support to the relay type devices. I think that's different than the remote_link support. I did a pull request ages ago that was rejected, so I just live with my local modifications.

@DoumP
Copy link
Contributor Author

DoumP commented Jan 13, 2016

Marc, I have fixed my patch so it's now clean and simple.
I think it should be in MisterHouse, but if it's not appropriate I'll do like gac410 and live with my local modification.

@marcmerlin
Copy link
Collaborator

@DoumP the problem is that your tree is not up to date, maybe it wasn't a checkout of master?
Either way, see how it says "This branch has conflicts that must be resolved"
I'm happy to help merge, but can't merge this, it won't work.

@hollie
Copy link
Owner

hollie commented Feb 25, 2016

This conflict is a result of the changes that were created by the perltidy pull request. Please see #566 on how to fix this.

Best regards,
Lieven.

@DoumP
Copy link
Contributor Author

DoumP commented Feb 26, 2016

It's not showing conflict anymore, I hope I did that the right way, if not I can just do a new pull request.

@marcmerlin
Copy link
Collaborator

something is off though
https://github.com/hollie/misterhouse/pull/478/files
shows me 0 files changed, 0 lines changed, i.e. you change seems empty

@DoumP
Copy link
Contributor Author

DoumP commented Feb 26, 2016

I've created a new pull request #572

@hplato
Copy link
Collaborator

hplato commented Mar 1, 2016

This has been superseded by a revised pull request.

@hplato hplato closed this Mar 1, 2016
@DoumP DoumP deleted the FastOn-FastOff branch March 7, 2016 23:31
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.

7 participants