Eria adurolight remote support

[mdolnik]

Refer to issues: #1730 and #3383

Refer to previous Pull Request #4211

These changes allow the Eria Adurosmart remote to send events for each button press.

From what I can tell there is no distinction from the remote for holding the button or double clicking, so this just provides simple button presses.

[mdolnik]

Please note the change in 4da5cc8 which is a little unconventional as the current logic is essentially ignoring the button map as this remote has a situation where it sends the same endpoint/cluster/command ID for every button pressed and only makes the distinction between buttons via payload value.

This switch has a situation where it sends the same endpoint/cluster/command ID for every button pressed and only makes the distinction between buttons via payload value.
The current logic in DeRestPluginPrivate::checkSensorButtonEvent() currently does not support this type of distinction, and the button map (field 5) only seems to support the first digit of the payload value and not the full payload value.
Therefore all of the information that distinguishes between the buttons ends up being ignored and the button events are being sent as the first mapped entry (button 1).
This commit changes the logic within DeRestPluginPrivate::checkSensorButtonEvent() to allow the value of the button pressed to be modified once a valid mapped entry has been found, and adds specific logic for the Eria Adurosmart Wireless Dimming Switch to change the button value based off of the payload value.

Ideally this type of situation should have an extra value (or modify the existing 5th value) in the button maps to allow for identification of the FULL payload to distinguish the types of buttons pressed.

I'm open to suggestions and/or I can help test any changes.

[mdolnik]

Also none of these changes allow it to be recognized by Phoscon (only deCONZ GUI).

The events can be picked up by Home Assistant in the /developer-tools/event page.

I'm not sure if this is normal, but it is added as a Device in Home Assistant, but not as an Entity.

[SwoopX]

I'm afraid all what's required is the creation of the button map and the addition of the manufacturer specific cluster at the right place, all other amendments are superfluous.

Also, the referred to PRs have the MFC VENDOR_JENNIC, so that would mean people running those would probably end up with non-working devices.

[mdolnik]

I'm afraid all what's required is the creation of the button map and the addition of the manufacturer specific cluster at the right place, all other amendments are superfluous.

The button map alone doesn't solve the problem as the following logic...

deconz-rest-plugin/de_web_plugin.cpp

Lines 4251 to 4260 in f3aec7b

	bool ok = false;
	for (const auto &buttonMap : buttonMapVec)
	{
	if (buttonMap.mode != Sensor::ModeNone && !ok)
	{
	if (buttonMap.mode == sensor->mode() &&
	buttonMap.endpoint == ind.srcEndpoint() &&
	buttonMap.clusterId == ind.clusterId() &&
	buttonMap.zclCommandId == zclFrame.commandId())
	{

...will ALWAYS select the FIRST button map entry EVERY time no matter what button is pressed. This is because the check above is only checking mode, endpoint, cluster ID, and command ID which is identical for every entry in this remote's button map. The only difference I am receiving in button presses is the payload value.

I believe this is the same issue as #4413.

Refer to this comment for the output of the No button map for logs for each button pressed on this remote.

If placing the "manufacturer specific cluster at the right place" could avoid this situation I am all for making the appropriate changes, but I would need to be shown where to make these changes as I could not find anywhere else that would translate specific payloads to different buttons.

Also, the referred to PRs have the MFC VENDOR_JENNIC, so that would mean people running those would probably end up with non-working devices.

Are you referring to changing { VENDOR_JENNIC, "Adurolight_NCC", jennicMacPrefix} to { VENDOR_ADUROLIGHT, "Adurolight_NCC", jennicMacPrefix}? This shouldn't really affect any existing usage as it was first introduced in #4211 which technically did not add support for this remote as the changes in that PR didn't work. #4211 was also the first mention of model ID Adurolight_NCC which was only made for this remote.

If you want, I can reverse commits 486ce64 and e403df9 which would return the vendor prefix back to JENNIC and make a special case for model Adurolight_NCC in DeRestPluginPrivate::addSensorNode() to avoid not being ignored because it is declared to have JENNIC prefix.

[SwoopX]

I stay brief here and apparently, I missed your feedback on the previous PR.

Your code is "just" working as it benefits from a logic flaw allowing incomplete button maps to be stored in memory. Incomplete in this sense because you added a 3 Bytes payload, which is disallowed and also throws errors in the debug log right after starting deconz. Apparently, a for-loop is not exited appropriately.

• As for the manufacturer code: The code a device has gets added, simple as that. However, the really problematic part I had in mind is obviously already gone.
• Regarding DeRestPluginPrivate::addSensorNode(): Here you're indeed right, a whitelisting is required. Doing whatever purely on the manufacturer code is just aching and an awkward legacy heritage, that hopefully will be eradicated in the mid term.
• Regarding the code at an appropriate place: Well, the place is right and now I understand why adding all that code was required to make it work (as mentioned at the top). In the end, it shouldn't be more than for the Sengled...

To not loop all the back and forth through, how about if I quickly put the required parts together and you check'em?

[mdolnik]

Your code is "just" working as it benefits from a logic flaw allowing incomplete button maps to be stored in memory. Incomplete in this sense because you added a 3 Bytes payload, which is disallowed and also throws errors in the debug log right after starting deconz.

Is this referring to the 000000, 000100, 000200, etc values in the button map? If-so those don't even need to be there as they never get read by any of the current logic as right now the for loop mentioned in the code reference of my previous comment will only grab the first button map entry every time.

I could technically change all of those values to 0 and/or only have one generic button map value that works for all buttons because the only thing in my pull request which is doing any actual button mapping is...

// Eria Adurosmart Wireless Dimming Switch.
else if (ind.clusterId() == ADUROLIGHT_CLUSTER_ID && sensor->modelId() == QLatin1String("Adurolight_NCC"))
{
    ok = true;
    // Since the only way to determine which buttons was pressed is by the payload,
    // change the value of buttonPressed based on the payload value.
    quint16 payload;
    QDataStream stream(zclFrame.payload());
    stream.setByteOrder(QDataStream::LittleEndian);
    stream >> payload;
    switch (payload) {
        case 0x000000: buttonPressed = (S_BUTTON_1 + S_BUTTON_ACTION_SHORT_RELEASED); break;
        case 0x000100: buttonPressed = (S_BUTTON_2 + S_BUTTON_ACTION_SHORT_RELEASED); break;
        case 0x000200: buttonPressed = (S_BUTTON_3 + S_BUTTON_ACTION_SHORT_RELEASED); break;
        case 0x000300: buttonPressed = (S_BUTTON_4 + S_BUTTON_ACTION_SHORT_RELEASED); break;
        default: ok = false; break;
    }
}

To not loop all the back and forth through, how about if I quickly put the required parts together and you check'em?

Thanks that would help. Are you implying that you will make your own pull request for this issue and I will review and ensure it works for me?

Regarding DeRestPluginPrivate::addSensorNode(): Here you're indeed right, a whitelisting is required. Doing whatever purely on the manufacturer code is just aching and an awkward legacy heritage, that hopefully will be eradicated in the mid term.

Fair enough, I will reverse commits 486ce64 and e403df9 (unless you're making your own PR).

[SwoopX]

Check out #4580

[mdolnik]

Check out #4580

I just tested the Eria remote with that branch and it works! Thanks for

I think I understand now how your approach works, essentially it loops through all of the button map entries and you're essentially setting the ok variable as to whether the current payload value matches the zclParam0 of the current button map entry.

I'm closing this PR in favor of your branch.

[SwoopX]

I guess the whole function is a bit... clumsy and could use a decent overhaul. I haven't dared that yet since mistakes would have quite some impact. It was already painful enough when I introduced the button maps due to other than obvious hidden dependencies.