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

Reduce code duplication and clean up the Inovelli Converter #8402

Merged
merged 9 commits into from
Dec 11, 2024

Conversation

rohankapoorcom
Copy link
Contributor

@rohankapoorcom rohankapoorcom commented Nov 29, 2024

In preparation for the new VZM30-SN, I spent some time cleaning up the existing converter, reducing duplication and moving as much as possible to use modern extends.

@InovelliUSA please review when you have some time.

The power / energy monitoring multipliers don't seem to be working currently (I commented on the specific line below).

electricityMeter({
current: false,
voltage: false,
power: {min: 15, max: 3600, change: 1},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The min/max/change here doesn't seem to be respected (I'm not sure why). When I run a reconfigure, it's added with activePower min: 10, max: 65000, change: 5 and currentSummDelivered min: 10, max: 65000, change: 0.1 instead of what's provided here (matching the custom call below)

Copy link
Contributor

Choose a reason for hiding this comment

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

There are two reporting mechanisms in the device that can sometimes overlap. When we first created the device we took our configuration options from our Z-Wave switch not knowing (at the time) that Zigbee had its own built in energy / power reporting functionality. So, you can have different configurations in the exposes tab vs the reporting tab. If I could do it again, I would remove config options 17-19 and only used the built in Zigbee reporting. Anyway, I think that is why you may be seeing this. Try setting P17-19 to 0 and then test the reporting configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I think my original message wasn't very clear.

I haven't changed any of the reporting parameters in the exposes tab. When the original code is running (see single commit diff: 44b0a9a), it explicitly sets (manually) the reporting to the min/max/change values as described on line 2025 and 2026. This leads to the following screenshot:

image

I switched the code to use the modern extend instead and passed it along the same min/max/change values as before. However, this doesn't seem to be taking affect. Leading to the following screenshot:

electricityMeter({
    current: false,
    voltage: false,
    power: {min: 15, max: 3600, change: 1},
    energy: {min: 15, max: 3600, change: 0},
})

image

I see duplication of the reporting calls (but with the correct values). Is that expected?

I think this is more of a question for @Koenkk.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, that isn't the question I thought it was and would be better answered by @Koenkk lol.

Copy link
Owner

Choose a reason for hiding this comment

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

The change you pass here is divided by the divisor, so the change is in W, see

change = property.change * (divisor / multiplier);

src/devices/inovelli.ts Show resolved Hide resolved
@Koenkk
Copy link
Owner

Koenkk commented Nov 30, 2024

@InovelliUSA please review

@Koenkk
Copy link
Owner

Koenkk commented Dec 3, 2024

Is this now ready for merge? (CC: @InovelliUSA )

@rohankapoorcom
Copy link
Contributor Author

rohankapoorcom commented Dec 3, 2024

From my side, I think there's still one question here: #8402 (comment) for you @Koenkk.

I'll let Eric comment if he has other concerns.

@InovelliUSA
Copy link
Contributor

Is this now ready for merge? (CC: @InovelliUSA )

Other than the question mentioned #8402 , I am ok with this merge.

@rohankapoorcom
Copy link
Contributor Author

@Koenkk are you good to merge this? I'd like to open the next PR (starting from this one) adding the new device (VZM30-SN).

Thanks!

@Koenkk
Copy link
Owner

Koenkk commented Dec 10, 2024

@rohankapoorcom yes, could you rebase it?

@rohankapoorcom rohankapoorcom force-pushed the clean-up-inovelli-converter branch from 640632d to 2419258 Compare December 10, 2024 19:51
@rohankapoorcom
Copy link
Contributor Author

Rebased @Koenkk

@Koenkk Koenkk merged commit 87e9228 into Koenkk:master Dec 11, 2024
2 checks passed
@Koenkk
Copy link
Owner

Koenkk commented Dec 11, 2024

Thanks!

@rohankapoorcom rohankapoorcom deleted the clean-up-inovelli-converter branch December 11, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants