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

Illuminance configuration improvements #468

Merged
merged 4 commits into from
Aug 22, 2019

Conversation

cdjackson
Copy link
Contributor

@cdjackson cdjackson commented Aug 10, 2019

This PR includes changes to the reporting configuration class ZclReportingConfig. This now supports changing the reporting change, and also the reporting min/max times.

@triller-telekom I'd welcome your review on this.

Note that there will be a subsequent PR to improve the handling of reporting configuration which is possible with the way reporting is now handled in the latest ZSS, but I'd like to get this merged as it already covers more than one feature.

Signed-off-by: Chris Jackson [email protected]

Copy link
Contributor

@triller-telekom triller-telekom left a comment

Choose a reason for hiding this comment

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

Indeed this PR does 2 things: fixing the illuminance formula and adding the possibility to change the reporting.

So I would rather vote to extract the illuminance fix out of this PR to have separate commits. WDYT?

@cdjackson
Copy link
Contributor Author

I would prefer not to have to waste time refactoring to be honest. The development of these features was interlinked which is why they ended up in a single branch and splitting them seems unnecessary even if in an ideal world we should only have one change per branch.

Is that ok? Otherwise it will be next week before I can look at this I think.

@cdjackson
Copy link
Contributor Author

Updated.

Copy link
Contributor

@triller-telekom triller-telekom left a comment

Choose a reason for hiding this comment

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

Sorry, I just found the time to do a further review.

As far as I understand the code, there CAN be a channel configuration specifying the reporting intervals. If there is none, the reporting uses the defaults as before. Am I right? Then, this PR contains the illuminance fix and a preparation of the channelconverter to support reporting intervals as a channel configuration, I am fine with that.

I just found 2 copy&paste mistakes, please have a look.

if (configuration.containsKey(CONFIG_REPORTINGMIN)) {
reportingTimeMin = ((BigDecimal) configuration.get(CONFIG_REPORTINGMIN)).intValue();
}
if (configuration.containsKey(CONFIG_REPORTINGMIN)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a copy&paste error here: should be CONFIG_REPORTINGMAX

reportingTimeMin = ((BigDecimal) configuration.get(CONFIG_REPORTINGMAX)).intValue();
}
if (configuration.containsKey(CONFIG_REPORTINGMIN)) {
reportingTimeMin = ((BigDecimal) configuration.get(CONFIG_REPORTINGCHANGE)).intValue();
Copy link
Contributor

@triller-telekom triller-telekom Aug 21, 2019

Choose a reason for hiding this comment

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

I think there is a copy&paste error here: should be CONFIG_REPORTINGCHANGE

Edit: I meant the line above, but I guess you know what I mean :)

@cdjackson
Copy link
Contributor Author

Then, this PR contains the illuminance fix and a preparation of the channelconverter to support reporting intervals as a channel configuration, I am fine with that.

No - I split the illuminance fix into #469

I'll update with the errors - well spotted, thanks.

Signed-off-by: Chris Jackson <[email protected]>
Copy link
Contributor

@triller-telekom triller-telekom left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM, now.

@cdjackson
Copy link
Contributor Author

Thanks Stefan

@cdjackson cdjackson merged commit c52e62a into openhab:master Aug 22, 2019
@cdjackson cdjackson deleted the illuminance branch August 22, 2019 07:24
@cdjackson cdjackson added this to the 2.5 milestone Dec 8, 2019
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.

2 participants