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

[New Device Support] IKEA E2123 #5583

Merged
merged 3 commits into from
Mar 23, 2023
Merged

[New Device Support] IKEA E2123 #5583

merged 3 commits into from
Mar 23, 2023

Conversation

ilueckel
Copy link
Contributor

This merge request contains a converter for the new IKEA Symfonisk Sound Remote.

The Github Issues Koenkk/zigbee2mqtt#16808 had quiet a discussion about wether one should use "light-remote" namings for actions or introduce "volume/sound - remote" ones.

I have chosen to use the toggle-converter from the previous symfonisk remote, but add new volume_<up|down>[_hold] and track_<previous|next> actions as the buttons on the remote a clearly meant for audio and not light control.

If you prefer to use already existing actions let me know.

Documentation on the main repository will be done in the coming days/after the merge request goes through.

@Koenkk
Copy link
Owner

Koenkk commented Mar 16, 2023

There is already a PR for this device: #5576

@MattWestb
Copy link

The #5576 is only working with factory shipped firmware and this is having working OTA ans the updated firmware is using Matter commands for the scene switch commands on manufacture cluster and having them on 2 extra endpoints.
This PR is working with both devices the linked not.

@junosuarez
Copy link

In addition to supporting both the stock firmware and the latest OTA update, this PR incorporates the discussion from
Koenkk/zigbee2mqtt#16808 around event naming for handling the volume up/down buttons.

The duplicated effort is unfortunate, but this PR appears to be the more complete of the two.

@junosuarez
Copy link

One open question - the stock firmware reports the dot buttons using the older zigbee "single, double, hold" event model. The new firmware appears to use a "button press, button release" event model.

What's the best way to expose these in this converter? Does zigbee-herdsman-converters have an opinion or preferred style, as a project? One option is to expose the events directly, another would be to use a common abstraction. The first option is "truer" to what the device is reporting, the second option is easier for downstream integrators to not have to care about what firmware is on the device.

Either way, consistency helps conceptual integrity.

@ilueckel
Copy link
Contributor Author

ilueckel commented Mar 16, 2023

To increase the complexity even more: For the dot - buttons the firmware 1.0.012 (20211214) only sends one event per single, double and hold click.

But firmware 1.0.32 (20221219) triggers:

  • Single Press: Initial Press, Short-Release (upon releasing the button)
 2023-03-16 21:25:34MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_initial_press'
 2023-03-16 21:25:34MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_short_release'
  • Double Press: Initial Press, Double Press
 2023-03-16 21:26:50MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_initial_press'
fo 2023-03-16 21:26:50MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_double_press'
  • Long Press: Initial Press, Long Press, Long-Release
2023-03-16 21:28:07MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_initial_press'
2023-03-16 21:28:07MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_long_press'
2023-03-16 21:28:09MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_long_release'

So Initial Press is always triggered, which is a bit of annoying :-/

@junosuarez I choose to not change the names of the v1 dot buttons as you suggested in Koenkk/zigbee2mqtt#16808 (comment) mainly to keep automations with these actions compatible after the firmware upgrade.

@ilueckel
Copy link
Contributor Author

@Koenkk I honestly didn't see that one coming from the discussion over at #5576

It seems the code in this PR already has some more iterations and testing done. The most significant difference would be that the code here supports the changed behavior of the remote after the firmware upgrade (where clusters, endpoints and actions change).

@junosuarez
Copy link

@ilueckel wow, what a fantastically annoying event model to program to :) I'm honestly thinking of just never upgrading my firmware once I get it working. With the two event models you showed, I don't see a way to make the event names compatible between the two versions. As you have it now in the code, the old firmware on a single click will send initial_press. However, in the new firmware, this gets sent at the beginning of both single and double click events. There's no way for consumer code to distinguish between a single or double click from a single event - rather, it must keep a window of recently received events to check for the absence of a double_press event. Like I said, super annoying!

Would doing what I described above be in-scope for a converter?

To avoid scope creep, my preferred approach is to land this initial PR and then make incremental improvements.

@ilueckel
Copy link
Contributor Author

It would work when you use dot_1_short_release for the single click in the V1 version. But that would be semantically wrong, as it would get triggered before releasing the button.

When using the V2 one should use the the _short_release action for single-button events.

These behaviors are definitly something to be noted in the docs of the remote.

As I said I would like to keep the actions compatible for both firmware versions. At the moment only the single press would be a breaking change.

@MattWestb
Copy link

The double is miss imprinted if looking in the Matter ZCL and IKEA have one bug.
Its starts with initial press and then shall being one multiples with variable how many pressed (command 5) that IKEA is not using and the last is multiples completed with with variable the number of the presses the finished (IKEA is doing it right its 2 for double) and no short release at the end.

It can being IKEA or other using more functions of the new scheme and doing it right you can with all commands getting getting all real multi functions and also variables for the switch new and old state.

I is only thinking doing it so its open for the rest functions and not doing somthing little between new and old way then its being better doing it all the way OK and its working good in the future.

@junosuarez
Copy link

@ilueckel in firmware v2, does short_release also fire after a second click? That's what I understand from #5583 (comment) . If so, then applications can't just handle short_release to mean "single click"

@ilueckel
Copy link
Contributor Author

I had to double check it again. Double click only publishes Initial Press & Double Press (no release action)

@junosuarez
Copy link

Wonderful, in that case it should work seamlessly as you have it.

const endpoint1 = device.getEndpoint(1);
const endpoint2 = device.getEndpoint(2);
const endpoint3 = device.getEndpoint(3);
await reporting.bind(endpoint1, coordinatorEndpoint, ['genOnOff', 'genLevelCtrl', 'genPollCtrl']);

Choose a reason for hiding this comment

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

For firmware v1, I think this also needs to bind 64639. Is there a way to tell at this point in the code whether it's v1 or not? If we include 64639 regardless, does it cause errors with firmware v2?

Choose a reason for hiding this comment

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

V1 is reporting OK on all cluster on EP 1 without bonding and V2 is doing the same but need EP 2 and 3 being bonded to the coordinator.

@Koenkk
Copy link
Owner

Koenkk commented Mar 18, 2023

Please let me know once this is ready for merge.

@pmfernandes
Copy link

Hi,

I've noticed that in this PR the "Play" button is using the toggle action instead of the track_play mentioned here: Koenkk/zigbee2mqtt#16808 (comment)

I've read the discussion about volume_up and volume_down and IMO if those buttons will have specific actions the "Play" should also have it's specific action.

If the intention is to adapat the action list to the remote purpose then you should consider the track_play.

@junosuarez
Copy link

@pmfernandes the problem with labeling it "play" is that if media is already playing, pressing it again will generate the same action - there is no way for the remote to know whether it is a "play" action or a "pause" action. I think toggle is a little more intention-revealing and less surprising to anyone consuming the event downstream.

@Koenkk @ilueckel this code looks good to me, all my concerns have been addressed. Let's get this merged!

@ilueckel
Copy link
Contributor Author

@Koenkk I also think this could be merged. There was no recent report with errors

@nielsrune
Copy link

@pmfernandes the problem with labeling it "play" is that if media is already playing, pressing it again will generate the same action - there is no way for the remote to know whether it is a "play" action or a "pause" action. I think toggle is a little more intention-revealing and less surprising to anyone consuming the event downstream.

@Koenkk @ilueckel this code looks good to me, all my concerns have been addressed. Let's get this merged!

I get your point, but since we already have track_next and track_previous, why not have the "Play" button named play_pause which IMHO I think is even more self explaining for a media controller?

This would correspond to the name of the HA service call: media_play_pause

@Koenkk Koenkk merged commit cbebe13 into Koenkk:master Mar 23, 2023
@Koenkk
Copy link
Owner

Koenkk commented Mar 23, 2023

Thanks!

@junosuarez
Copy link

@nielsrune I have no objections to naming it media_play_pause - if you want to pursue that, it'd be best to open a new PR or issue to discuss further.

@Skaronator
Copy link
Sponsor Contributor

Skaronator commented Mar 31, 2023

Just a quick report (I'll do an issue later): Since updating the firmware, the dot actions are not working. Neither initial press, double press nor long press are working at all. Everything all works fine.

I'm on the latest dev branch release.
ghcr.io/koenkk/zigbee2mqtt:latest-dev@sha256:2c33571c3882ee55fadce96a97045d6cb312cbd857e9ab0fd03eded58a47f26a

{
    "battery": 100,
    "last_seen": "2023-03-31T13:42:34+02:00",
    "linkquality": 58,
    "update": {
        "installed_version": 16777266,
        "latest_version": 16777266,
        "state": "idle"
    },
    "action": "volume_up",
    "update_available": null
}

@DannyDeGaspari
Copy link

See Koenkk/zigbee2mqtt#17142

@Skaronator
Copy link
Sponsor Contributor

Awesome, I'll try it later. Thanks!

@antonio1475
Copy link

antonio1475 commented Apr 14, 2023

To increase the complexity even more: For the dot - buttons the firmware 1.0.012 (20211214) only sends one event per single, double and hold click.

But firmware 1.0.32 (20221219) triggers:

  • Single Press: Initial Press, Short-Release (upon releasing the button)
 2023-03-16 21:25:34MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_initial_press'
 2023-03-16 21:25:34MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_short_release'
  • Double Press: Initial Press, Double Press
 2023-03-16 21:26:50MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_initial_press'
fo 2023-03-16 21:26:50MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_double_press'
  • Long Press: Initial Press, Long Press, Long-Release
2023-03-16 21:28:07MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_initial_press'
2023-03-16 21:28:07MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_long_press'
2023-03-16 21:28:09MQTT publish: topic 'zigbee2mqtt/Sonos Fernbedienung Wohnzimmer/action', payload 'dots_2_long_release'

So Initial Press is always triggered, which is a bit of annoying :-/

@junosuarez I choose to not change the names of the v1 dot buttons as you suggested in Koenkk/zigbee2mqtt#16808 (comment) mainly to keep automations with these actions compatible after the firmware upgrade.

Hi @ilueckel and @junosuarez,

Regarding the long & double dot press, isn't this very much like what happened with the E2002 and was solved thanks to Koenkk/zigbee2mqtt#13335, specifically Koenkk/zigbee2mqtt#13335 (comment) by @Koenkk (original issue Koenkk/zigbee2mqtt#9867)?

From what I can see/understand on those linked issues, a delay was added in the converter to differentiate between single/double/long

Thank you 😄

@MattWestb
Copy link

Nop the 1.0.12 firmware is sending unicast and only one command then you is pressing the shortcut button. The 1.0.32 and 1.0.35 is using Matter standard commands (90% compatible) and the initial press can being ignored if you dont like it but its there and can being used if you like and all is unicast and only one command is sent / button pressing.

TF have putting 1.0.35 in its OTA feed for some days ago so all that is updating from 1.0.12 need deleting the remote and adding it new so the system is reading the new endpoints for the shortcut buttons and binding them or is they not working.

@antonio1475
Copy link

Nop the 1.0.12 firmware is sending unicast and only one command then you is pressing the shortcut button. The 1.0.32 and 1.0.35 is using Matter standard commands (90% compatible) and the initial press can being ignored if you dont like it but its there and can being used if you like and all is unicast and only one command is sent / button pressing.

TF have putting 1.0.35 in its OTA feed for some days ago so all that is updating from 1.0.12 need deleting the remote and adding it new so the system is reading the new endpoints for the shortcut buttons and binding them or is they not working.

Apologies, I'm not sure I understood you, nor that I was clear on my last sentence (which I've now edited).

My point was that on the E2002, when long-pressing the side buttons (arrows), an "on" payload was first sent (equivalent to pressing the up button), and then the arrow_left_hold.

In other words, holding a side (left/right) arrow also sent the "up" (on) command first.

Now for E2123 is very similar: when holding or douple pressing a dot, the dots_2_initial_press is first sent.
So, if you want to map 3 different actions to dots_2_initial_press, dots_2_long_press and dots_2_double_press, dots_2_initial_press will always happen when doing long or double.

In my opinion, by doing the same as was done for E2002 (adding a delay to of ~700ms to see if a side arrow was pressed or not before accepting the "on" payload), we can have the 3 mappings for the dots:

Wait X ms to see if dots_2_long_press or dots_2_double_press before accepting dots_2_initial_press. If dots_2_long_press or dots_2_double_press is received, do those. If not, do dots_2_initial_press

I will try to work on an external converter based on Koenkk's, but it's pretty far from my knowledge...

@MattWestb
Copy link

Way is you not using dots_2_short_release, dots_2_double_press (wrongly named shall being release or multi 2) and dots_2_long_press ?
You dont need using the other functions for your user case.

Styrbar is completely different then long press right or left is sending one reset / sync sequence to bonded devices for getting them in sync and its sending very many commands that is used for other function and making problems.

@antonio1475
Copy link

I see your point.
I was actually working on a blueprint right now and doing that. However, the delay between dots_2_initial_press and dots_2_short_release is 750-850ms, while the difference between dots_2_initial_press and dots_2_double_press is ~400ms, and ~250ms for dots_2_long_press.

Not a big deal for one action, but noticeable/slow for repeated things (eg. I'm going to control a speaker group volume with the +/-, while controlling the volume of the 2 speakers independently with the dots), so I'll be repeating clicks.

Anyway, I'll share if I reach anything good.

@MattWestb
Copy link

Way not using the up and down long press for volume then its sending multiple command as long you is holding then down ?

And i dont understanding you mili secs then you is getting the commands pure from the device and if you like making other times use the dots_2_long_press and and measuring how long it taking to you is getting the dots_2_long_release but if i understanding you need 20 more function from the device that is not made for doing it so you is on you own then.

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.

9 participants