-
Notifications
You must be signed in to change notification settings - Fork 363
genTime: Respond with default value for daylight saving time-related fields #1515
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
genTime: Respond with default value for daylight saving time-related fields #1515
Conversation
…ft and standardTime in genTime field
| }); | ||
|
|
||
| it("Respond to genTime read", async () => { | ||
| const frame = Zcl.Frame.create(0, 0, true, undefined, 40, 0, 10, [{attrId: 0}, {attrId: 1}, {attrId: 7}, {attrId: 4}], {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Koenkk : Is there a reason for the dstEnd field being requested here, but not expected that to be there later on (expect(message.payload.length).toBe(3))?
|
About the dst attributes, they are optional. Could create friction with some devices that don't support them. The time cluster appears to be calculating |
…es and 0 isn't the correct answer here logically
|
As discussed in Koenkk/zigbee-herdsman-converters#10110 (comment), the value that needs to be returned seems to be too specific, let's use the |
That makes sense. 👍 About general support for daylight saving time in the future: What is your opinion about using third-party libraries, specifically https://github.com/SpiritIT/timezonecomplete for that? Just that I don't waste time. 😄 |
|
@DerDreschner it's a quite bulky dependency (1.85MB) (https://www.npmjs.com/package/timezonecomplete), so would not use it. |
|
We probably should look into replacing the moment library from Z2M deps, it seems to be pretty much abandoned (2023), and it's very bulky for what we use it for (2 calls...):
This might be a good time to look for the features needed for this as well. There are some refs on moment website, but I haven't looked into what's the "most popular" successor. |
|
Yeah, I've considered moment-timezone first (timezone focused extension for moment), but it's in maintenance mode as well. Considering the combined size of moment and moment-timezone (4.35MB and 2.85MB), timezonecomplete looks pretty slick to me.
Yeah, I've checked the successors mentioned on their website. But 3 out of the 4 mentioned libraries (that's all I've checked so far, besides timezonecomplete) don't support use cases like "give me the moment of the next dst for that timezone", "does that timezone have a dst at all?", "what's the shift to utc not considering dst" etc. There's always something missing. But these two calls could be done with timezonecomplete as well, so, maybe that is a possible solution? |
|
I'm not very familiar with the time cluster or what's needed here, rarely used it, but we probably should look at a more established lib. |
Yeah, but as we only send the attributes that are requested (and therefore supported), this shouldn't be an issue in my opinion.
Both Only Lets continue that discussion in #1517. |
The Bosch devices ask the coordinator for information about daylight saving time. Without any answer, the device still asks regularly, which drains the unncecessarily drains the battery. Therefore, I've added an appropriate
fromZigbeeconverter for that case.During testing for Koenkk/zigbee-herdsman-converters#10110, I realized that this solution doesn't work anymore in Z2M Edge as zigbee-herdsman doesn't forward the request to zigbee-herdsman-converters. To solve that, I've rewritten my solution to a
customReadResponse. But @Koenkk made the suggestion to solve it here.I'm not sure if my solution is okay, but I just answered the missing fields with their default value according to the Zigbee Cluster Library Specification, Revision 8:Okay, the device doesn't shut up asking with the default value. It has to be 0x00.