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

Update frient powermeter led 2 #8044

Merged
merged 14 commits into from
Oct 16, 2024
Merged

Conversation

tbowmo
Copy link
Contributor

@tbowmo tbowmo commented Sep 28, 2024

This should fix Koenkk/zigbee2mqtt#19042

src/devices/frient.ts Outdated Show resolved Hide resolved
src/lib/develco.ts Outdated Show resolved Hide resolved
src/devices/frient.ts Outdated Show resolved Hide resolved
@tbowmo
Copy link
Contributor Author

tbowmo commented Oct 5, 2024

@Koenkk I have tested the integration locally, and it works, at least I get energy etc. from the device

{
    "battery": 100,
    "battery_low": false,
    "energy": 118.28,
    "interface_mode": "electricity",
    "linkquality": 112,
    "power": 421,
    "power_2": 357,
    "pulse_configuration": 1000,
    "update": {
        "installed_version": 196866,
        "latest_version": 196866,
        "state": "idle"
    },
    "voltage": 3100,
    "current_summation": null,
    "update_available": null
}

const multiplier = msg.endpoint.getClusterAttributeValue('seMetering', 'multiplier') as number;
const divisor = msg.endpoint.getClusterAttributeValue('seMetering', 'divisor') as number;
const multiplier = (msg.endpoint.getClusterAttributeValue('seMetering', 'multiplier') ?? 1) as number;
const divisor = (msg.endpoint.getClusterAttributeValue('seMetering', 'divisor') ?? 1000) as number;
Copy link
Contributor Author

@tbowmo tbowmo Oct 5, 2024

Choose a reason for hiding this comment

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

@Koenkk one thought here, could it be an idea to convert this to Number, instead of doing as number?

like this instead

const divisor = Number(msg.endpoint.getClusterAttributeValue('seMetering', 'divisor') ?? 1000);

(and likewise for the multiplier of course)

Copy link
Owner

@Koenkk Koenkk Oct 5, 2024

Choose a reason for hiding this comment

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

This value is always a number (or undefined) so it should not be needed. Thinking about this again, I'm not sure about these default values, why did you add them? By default it just publishes the raw value in case there is not multiplier/divisor

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 had problems, because they where undefined, and so factor became null. Then on line 756, it checks if factor is not null, before calculating anything. That lead up to it reporting null as energy consumption.

When I added those default values, it was reporting like it should.

Copy link
Owner

Choose a reason for hiding this comment

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

The divisor/multiplier can be forced in the configure. I would propose to have the same logic as instantaneousDemand here, expose raw value when no factor is defined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm.. even though that I defined the metering entity like

  electricityMeter({cluster: 'metering', power: {divisor: 1000, multiplier: 1}, energy: {divisor: 1000, multiplier: 1}}),

I had trouble in the fromZigbee.metering function. This was the previous release though, so things might have changed now.

Anyways, I have changed the logic a bit, so it now returns the raw value.. I'll get around to test things in a couple of days.

src/devices/frient.ts Outdated Show resolved Hide resolved
@@ -170,4 +170,26 @@ export const develcoModernExtend = {
valueIgnore: [0xffff, -0x8000],
...args,
}),
current_summation: (args?: Partial<NumericArgs>) =>
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
current_summation: (args?: Partial<NumericArgs>) =>
currentSummation: (args?: Partial<NumericArgs>) =>

valueMax: 268435455,
...args,
}),
pulse_configuration: (args?: Partial<NumericArgs>) =>
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
pulse_configuration: (args?: Partial<NumericArgs>) =>
pulseConfiguration: (args?: Partial<NumericArgs>) =>

@tbowmo
Copy link
Contributor Author

tbowmo commented Oct 16, 2024

@Koenkk I have now tested the implementation, and it works. Both changing pulse counts, and setting current summation property.
So it's a go for a merge from me now.

@Koenkk Koenkk merged commit 6e84179 into Koenkk:master Oct 16, 2024
2 checks passed
@Koenkk
Copy link
Owner

Koenkk commented Oct 16, 2024

Thanks!

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.

[New device support]: EMIZB-141 Power Meter
2 participants