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

Add units to Illuminance and Relative Humidity, fix missing units #765

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lucasec
Copy link
Contributor

@lucasec lucasec commented Jun 6, 2022

This PR should include all code in #597, plus a few other places I found the units missing in dynamically defined channels.

It adds new units to the following channel types:

  • Relative humidity is now defined as a Number:Dimensionless
  • Illuminance is now defined as a Number:Illuminance

Since the addition of units may require things to be deleted/re-created, this may be a breaking change.

@lucasec lucasec mentioned this pull request Jun 6, 2022
Copy link
Contributor Author

@lucasec lucasec left a comment

Choose a reason for hiding this comment

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

Adding a few open questions I have

@@ -145,7 +145,7 @@ public Channel getChannel(ThingUID thingUID, ZigBeeEndpoint endpoint) {
return ChannelBuilder
.create(createChannelUID(thingUID, endpoint,
ZigBeeBindingConstants.CHANNEL_NAME_ELECTRICAL_ACTIVEPOWER),
ZigBeeBindingConstants.ITEM_TYPE_NUMBER)
ZigBeeBindingConstants.ITEM_TYPE_NUMBER_POWER)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming when we dynamically generate channels, e.g. for a generic device without a thing XML definition, we ignore the contents of channels.xml completely? Thus without changing the type here we were not actually getting the correct channel type?

Copy link
Contributor

Choose a reason for hiding this comment

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

No - the channels XML file is not ignored. The channel type definitions are still required. When you define a channel (either programatically or in the thing XML file) you have to specify the channel-type, but the channel can also override certain aspects of this. I'm pretty sure that if you try and define a channel that doesn't have a channel type defined, it will error.

.setReporting(REPORTING_PERIOD_DEFAULT_MIN, REPORTING_PERIOD_DEFAULT_MAX, 50).get();
handleReportingResponse(reportingResponse, POLLING_PERIOD_DEFAULT, REPORTING_PERIOD_DEFAULT_MAX);
CommandResult reportingResponse = attribute.setReporting(reporting.getReportingTimeMin(),
reporting.getReportingTimeMax(), reporting.getReportingChange()).get();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not exactly clear on the lifecycle order—would the CHANGE_DEFAULT configured on line 115 be picked up here? As in the converter is initialized first then when we create the reporting it reads the already defined default parameter values?

Copy link
Contributor

Choose a reason for hiding this comment

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

No - it won't - it will use whatever the default is in the ZclReportingConfig class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming the device does not immediately get reconfigured later with the actual default either (unless the user goes in and changes/saves the channel options)?
So I probably need to refactor the pattern that was used in other converters somewhat to ensure it uses a reasonable default

@cdjackson cdjackson added this to the 3.4 milestone Jun 29, 2022
@kaikreuzer kaikreuzer removed this from the 3.4 milestone Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants