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

Events of remotes are fired multiple times #2

Open
hfedcba opened this issue Jan 4, 2016 · 9 comments
Open

Events of remotes are fired multiple times #2

hfedcba opened this issue Jan 4, 2016 · 9 comments

Comments

@hfedcba
Copy link
Contributor

hfedcba commented Jan 4, 2016

First of all: Really nice project 👍!

During my first test with a remote, I noticed some small issues: When I press a button on my remote (HM-RC-Key4-2), I get two "press_short: true" events by the Homegear node (probably one for press_short and one for install_test?). Maybe it would be better to only put the value received over MQTT into the property object? Alternatively not received action values could be set to "false" in the property object? Using "Only send a message on property changes" is not working here, as the variables are of type action and always true. Also PRESS_CONT and PRESS_LONG_RELEASE would be nice 😋. And: Key 3 and 4 are ignored.

Cheers,

Sathya

@HealsCodes
Copy link
Owner

Thanks for giving it a good test ride (I can only test against my RT-CC-DN thermos).

Maybe you can assist a little with this issue if you like:

The "Only changes" mode was supposed to also work in this scenario but I can't test it against a remote since I have none. Setting not received actions to false would probably be a nice idea - I'll think about it.

Could you use a tool like mqtt-sub to listen to the events published by Homegear and send me an excerpt showing the sequences for when you press and when you release the button?

Regarding the missing keys - It's possible that some keys are missing because the conversion script didn't find the correct number of channels in the source XML (some devices don't supply a fixed channel number but rather a "check device at runtime" flag, in this case my conversion script used a little heuristic to try to guess the channel count from the name).

Feel free to check out the device description in families/homegear.json and submit a PR.

@hfedcba
Copy link
Contributor Author

hfedcba commented Jan 4, 2016

Here's the log of one continuous button press.

homegear/1234-5678-9abc/event/45/0/RSSI_DEVICE [-71]
homegear/1234-5678-9abc/event/45/1/INSTALL_TEST [true]
homegear/1234-5678-9abc/event/45/1/PRESS_CONT [true]
homegear/1234-5678-9abc/event/45/1/PRESS_LONG [true]
homegear/1234-5678-9abc/event/45/1/TEST_COUNTER [132]
homegear/1234-5678-9abc/event/45/1/SIM_COUNTER [132]
homegear/1234-5678-9abc/event/45/1/INSTALL_TEST [true]
homegear/1234-5678-9abc/event/45/1/PRESS_CONT [true]
homegear/1234-5678-9abc/event/45/1/PRESS_LONG [true]
homegear/1234-5678-9abc/event/45/1/TEST_COUNTER [132]
homegear/1234-5678-9abc/event/45/1/SIM_COUNTER [132]
homegear/1234-5678-9abc/event/45/1/INSTALL_TEST [true]
homegear/1234-5678-9abc/event/45/1/PRESS_CONT [true]
homegear/1234-5678-9abc/event/45/1/PRESS_LONG [true]
homegear/1234-5678-9abc/event/45/1/TEST_COUNTER [132]
homegear/1234-5678-9abc/event/45/1/SIM_COUNTER [132]
homegear/1234-5678-9abc/event/45/1/INSTALL_TEST [true]
homegear/1234-5678-9abc/event/45/1/PRESS_CONT [true]
homegear/1234-5678-9abc/event/45/1/PRESS_LONG [true]
homegear/1234-5678-9abc/event/45/1/TEST_COUNTER [132]
homegear/1234-5678-9abc/event/45/1/SIM_COUNTER [132]
homegear/1234-5678-9abc/event/45/1/INSTALL_TEST [true]
homegear/1234-5678-9abc/event/45/1/PRESS_CONT [true]
homegear/1234-5678-9abc/event/45/1/PRESS_LONG [true]
homegear/1234-5678-9abc/event/45/1/TEST_COUNTER [132]
homegear/1234-5678-9abc/event/45/1/SIM_COUNTER [132]
homegear/1234-5678-9abc/event/45/1/INSTALL_TEST [true]
homegear/1234-5678-9abc/event/45/1/PRESS_CONT [true]
homegear/1234-5678-9abc/event/45/1/PRESS_LONG [true]
homegear/1234-5678-9abc/event/45/1/TEST_COUNTER [132]
homegear/1234-5678-9abc/event/45/1/PRESS_LONG_RELEASE [true]
homegear/1234-5678-9abc/event/45/1/SIM_COUNTER [132]

PRESS_LONG and PRESS_CONT are published roughly every 200 ms for the duration of the button press.

@HealsCodes
Copy link
Owner

Thanks, that's going to help sorting this out.
I'm not 100% sure how to handle this situation in a way that doesn't break the current "parameter set" design (which I really like to keep).

Extending your proposal to send all other actions as false if a new action is received as true would work here but it would still send more than one message in short succession (and then repeat them):

  • install_test -> true, others false
  • press_long -> true, others false
  • press_cont -> true, others false

It would also require adding more metadata to the device description since currently receive parameters are handled without any type information (only write params have them). That could be extracted from the source XML so it's not that big a change but it would still be a necessary one.

@hfedcba
Copy link
Contributor Author

hfedcba commented Jan 4, 2016

I made the pull request and noticed channel 3 was already there - twice. The XML file contains definitions for all 4 channels. It seems there went something wrong during the conversion.
Regarding the dynamic channel count: This is difficult to handle statically. But if you send me a list of devices you need the channel count for, let me see what I can do (I should be able to get the channel count for most devices). Alternatively you could just accept all channels when there is a dynamic count? Or is that difficult to handle?

@hfedcba
Copy link
Contributor Author

hfedcba commented Jan 4, 2016

If you want to keep the "parameter set" design, I don't think there is a good way to avoid sending multiple successive messages. But I also don't think that is a problem.

@HealsCodes
Copy link
Owner

Knowing the actual channel count is required in two places:

  • for the homegear-mqtt-out node to be able to display the supported parameters to send the value to
  • for the homegear-mqtt-in node it's required to build the parameter set dictionary

Eliminating the first one is something I don't think can be done easily without sacrificing the UI comfort.
The second one could actually be replaced by a version of auto-discovery which would use the result of getAllValues to determine what should be in a complete parameter dict.
Will getAllValues really supply all readable parameters (including actions etc.)?

@hfedcba
Copy link
Contributor Author

hfedcba commented Jan 4, 2016

Yes, getAllValues returns really all readable parameters.

@HealsCodes
Copy link
Owner

Alternative approach if the device has readable actions:

  • add a second output-port which will send a message containing the action name on trigger
  • exclude these actions from the parameter set send out by the normal output

This operation mode could also have a separate option to enable it.

@hfedcba
Copy link
Contributor Author

hfedcba commented Jan 7, 2016

I like the first option, so you always have all data and the name of the triggered variable.

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

No branches or pull requests

2 participants