-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[WIP] weekly_schedule expose #5550
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
Oh neat, I guess the main error was the typo in composite. Using an enum is indeed a nice solution for the fix set of options. I got a bit further but hitting some new problems now:
I pushed the commit (for now only on the device I'm testing with) so you can see what I have so far, once it works I think it probably should be moved to the climate() class as a 'withWeeklySchedule()` Screen.Recording.2023-03-05.at.11.25.03.move.g. generated payload if I race like a mad man before the frontend resets
|
19e06ff
to
6f4bde2
Compare
Could it be that the device publishes it state when this happens? (can you provide the debug log when this happens)
min/max is indeed ignored by the frontend. |
There is certainly stuff being publish when this is happening, but from other devices (trv/office is the one I had open on the exposes tab on the frontend):
I had it reset the view a few times, this was the activity during that time. Firefox also showed no requests or messages on the webdeveloper console when this is happening.
|
Seems similar to Koenkk/zigbee2mqtt#16940 |
Yep that looks like the exact same problem. So I think that just leaves:
|
I think it will be better to use two input number fields (hours and minutes) |
Then we can't have the attribute accept both str and int though Edit: Well i can make it accept int,str and object with little extra work, that probably a nice way to do it |
OK we now accept either:
|
@Koenkk aside from the frontend bug currently making this usable because it keeps resetting... there is still the dayofweek issue where the list ends up like Maybe this is another bug on how the frontend handles it? |
@Koenkk more interesting behavior :( I moved everything over to Climate() and now the frontend doesn't render an apply button! Here are the respective payloads the frontend sees:
and
Aside from the name they are identical! The _old variant was generated by:
The other one by the new code in this PR. |
Probably yes, @nurikk do you have time to look into this? |
@Koenkk Still seems pretty broken, it seems to 'reset' less though.
Here is the debug log
Screen.Recording.2023-03-20.at.20.09.45.mov |
Hi! |
And btw, according to exposé’s definitions enum has to have “key” attribute. Exposes specification doesn’t support primitive types other than “number” for array type. |
@Koenkk I guess we can't expose dayofweek as a list of enum then. Any other way to describe this? Or should something new be added? |
|
you just add key='day' to your enum definition. then it will become |
Wrapping it in another composite abstraction gets rid of the undefined but all other mentioned bugs are still present. It's also a bit silly and a lot of extra code that we really shouldn't be needing with all the additional composites. I feel like a list of str (based on an enum) seems like something we should somehow support. As described above there is not 'apply' button for the weekly_schedule level, the seperate apply button in the nested composites trigger incomplete (and thus invalid) payloads. Also by the time you have your desired payload a bunch of broken ones are queued up that are all sent once the device wakes up. I managed to crash my test TRV. |
@Koenkk I think with the workaround by adding an extra abstraction level for the daysofweek this now works, aside form the bugs in the frontend that are still present? Or am I missing something else. |
hi @sjorge , can you please try following version. |
Will try this version tomorrow, going to be a long day at work today. |
…9, "minute": 30} object This so that we can make actually have it described somewhat as expose data as we have no 'time' or 'datetime' picker.
This is getting redicilous, maybe some additions to the exposes format are needed.
Looking good so far:
Only published once I hit apply (which is what is expected) Screen.Recording.2023-05-02.at.20.16.23.movSo LGTM |
I also did a few more tries while sending get requests for other attribute so messages for the device are arriving, the interface does not seem to reset so thats also good. |
@Koenkk should be mergable now that we have Koenkk/zigbee2mqtt@b8e4607 |
Cool thanks again! |
@sjorge is my understanding correct in that this schedule functionality allows us to only define a number of schedule transitions that can be applied to one or more days.... but it is not currently possible to define different transitions for different days? The Sonoff TRVs (and the ZCL specification) allow a different schedule to be configured for each day, so I guess I'd be looking for something similar to this:
Was this something you ever considered? |
Yeah, I've seen multiple schedules working via MQTT, as you so describe - however, it looks the ability to do this in the frontend is lacking, if I'm not mistaken. I haven't yet seen a way to define multiple schedules in the UI. Just considering whether it's worth changing the frontend to do this... |
zigbee-herdsman-converters/src/converters/toZigbee.js Lines 1229 to 1251 in f3d6d59
It was already very hard to get it to work in the frontend at all :( You can in theory apply one, refresh the page and apply the 2nd one, .. repeat I just use it over MQTT and clear + set + set, everytime I want to change something |
My Sonoff TRVs only report back the schedule when the device is first paired and when the schedule is modified for a given day. The schedule is reported back in a series of messages, one per day, with each day having its own schedule. This obviously causes issues with the state in Z2M - Maybe I'll look at it, maybe I won't...it's not a feature I will personally use anyway (I use Home Assistant for scheduling) - however I always like a challenge. (And perhaps it might be useful to some people out there who use Z2M simply as a hub replacement rather than integration with HA, if there are indeed any!). |
Since we now calculate the derived values, this should make the exposes data simpler.
The following now works correctly: