HA discovery changes

[robertalexa]

Hi @Koenkk and @sjorge

A couple of things committed, plus another not yet because I wanted to talk to you before.

1. .gitignore .idea IDE folder (get this out the way)
2. remove redundant comment. This is from when we have split the binary attributes by SET access.
3. split the numeric attribute from sensor only to sensor and number, if the attribute supports SET.
   One thing to ask you:
   

I have set the min and max values (otherwise they default to 0 and 100) as defined in the expose (withValueMin/Max), but I had to also do a fallback for attributes that do not use fixed limits, or they actually use the last available hex. For example:

Do you have any issues with that?

On top, it should not affect any of the devices in lookup as I think they do not support get anyway. But maybe I am overseeing sth? scrap that. I was referring to unit of measurements etc which belongs to a device class, which is not supported by number entities, but I don't see that being a problem

I have tested it and it behaves as expected, both read and write work fine, both non-set and set attributes gets exposed correctly. Here is my TRV:

As you can see battery or load_extimate are sensors while others are numbers.

And now onto what I have not done yet, but I think i should?

enum attributes currently get a duplicate if they allow SET. This is the current state of the code:

As you can see, the sensor entity is disabled by default if the attribute supports set, and a new attribute is created instead as a select.

Which you can see in my TRV screenshot above where i have day_of_week as a select but also disabled at the bottom.

Is there any reason why this has this approach instead of either sensor or select?

Thanks

[Koenkk]

Is there any reason why this has this approach instead of either sensor or select?

The reason to discover both is to prevent a breaking change.

This PR will also be a breaking change, note that this PR removes some discovered sensors (the ones that are now discovered as number).

I suggest to:

• Discover both sensors
• For the sensor add to the discovery_payload: enabled_by_default: !(firstExpose.access & ACCESS_SET). In this way both sensors get discovered but for new discovered sensor the sensor is disabled when it also has a corresponding number discovery.

[robertalexa]

Hi @Koenkk and thanks for the feedback. A while ago we have done the same for the binary type and this was not flagged at that time.

This PR will also be a breaking change, note that this PR removes some discovered sensors (the ones that are now discovered as number).

I understand what you mean, but equally i don't fully agree. I will explain my logic, and if you still think the "breaking change" is indeed that important I will re-write the code to your suggestion.

• If a current attribute is numeric and DOESN'T have set -> it gets exposed as a sensor. User can't do anything more than read that value from it. This remains the same after my PR
• If a current attribute is numeric and DOES have set -> it will not expose the sensor anymore (which was read only anyway). Instead it will expose a number, which the user can both read and write to.

Your logic implies that someone relies on a value that COULD be written to, but solely for reading purposes, and use a different method to write it rather than via HA entity. So after your suggestion, because the entity is now disabled by default, it will still break. Only that instead of the user changing their "automations", they just enable a "duplicate" entity instead of reading from the same one they could also write to.

Feels counter-intuitive and duplicates data (in HA).

Let me know what your thoughts are, if you still think your approach is healthier, just let me know and I will make amends :)

Thanks.

PS: i can see @sjorge also agrees with you

[sjorge]

PS: i can see @sjorge also agrees with you

Sort of, I agree breaking existing user's setups is to be avoided. But I don't use HA so I am not sure if swapping something expose to HA that is readonly numeric to a settable numeric with the same name will break things or not.

Logically I don't think it will, but HA is a complex beast so idk.

[Koenkk]

@robertalexa but if I understand correctly, the entity_id changes right? So if an automation is currently using a sensor which is replaced by a number it will be broken.

[robertalexa]

@Koenkk Yes that is correct, and that i can 100% agree with. What I am trying to say is, chances of someone reading from a SET attribute, and then SETting it using mqtt (since HA is just sensor), to me suggests a competent person :) and that person can fix their stuff pretty fast :)

Breaking changes do happen, it is in the nature of code, but if you are to ask me what I prefer, I would always answer good clean code with as little duplication as possible. So I would vote for a breaking change as long as it is announced and documented. Otherwise you compromise the future for the sake of the past :)

But I will do whatever you tell me to, it is your project, you set the direction :)

Say the word and I go and correct my code.

PS: i don't mean to try to persuade any of you, I am honestly just looking for opinions and choose the most future-proof solution. Both solutions would work. 1 will create duplication for 100% of the users, the other will create a problem for a much smaller % of the users (the automaters) - users = people that have devices already paired. new paired devices are not affected

[sjorge]

We do have the legacy flag, can that be used to indicate to keep the old numeric or expose the new one? (I mean technically it can, but is it a good idea)

[Koenkk]

@robertalexa lets keep discovering both, note that zigbee2mqtt follows semver, and thus a breaking change would involve bumping the major version. For sure I want to release z2m 2.0.0 sometime but there are some other breaking changes that I also want to do.

As long as you add a comment above it explaining it is legacy it's fine by me.

[robertalexa]

Apologies man, I have been out all day. I have taken notes of your explanation and will change my code to match the request. Will probably do that tomorrow and let you know. Thanks for taking the time to talk this through. Much appreciated :)

[robertalexa]

@Koenkk I have now updated the code as per our discussion. There are now errors, but not introduced by the changes - see CI results.

Just to validate, I have locally reverted my changes completely, ran npm run test-with-coverage and the issues persists.

3. split the numeric attribute from sensor only to sensor and number, if the attribute supports SET.
One thing to ask you:

I have set the min and max values (otherwise they default to 0 and 100) as defined in the expose (withValueMin/Max), but I had to also do a fallback for attributes that do not use fixed limits, or they actually use the last available hex. For example:

Also, any issues with the above?

LE: the CI seems to be as a result of me merging your latest commit into my branch

[Koenkk]

The test failed because the added enabled_by_default attribute in ff83c9c#diff-5cd90a075b18a077fab2f7f5a4066f63eade9d49e5ef0b023bc3878272786e9eR484, I've updated the tests with this.

Thanks!