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

[Refactoring][Part 2] Updating Lumi (Aqara and Xiaomi) specific converters #6982

Merged
merged 16 commits into from
Feb 11, 2024

Conversation

mrskycriper
Copy link
Contributor

@mrskycriper mrskycriper commented Jan 31, 2024

TODO

  • move Lumi related converters from lib/legacy.ts, converters/fromZigbee.ts and converters/toZigbee.ts to lib/lumi.ts
  • Update converter usages in devices/lumi.ts
  • Rename converters to lumi_*
  • Combine device specific converters into generic ones
  • Verify, what to do with legacy converters

Questions and thoughts

  • Can legacy functions be updated?
  • Should they be updated?
  • It looks like some older devices will not work without legacy mode.

@mrskycriper mrskycriper marked this pull request as draft January 31, 2024 00:06
@Koenkk
Copy link
Owner

Koenkk commented Jan 31, 2024

Can legacy functions be updated?

yes

Should they be updated?

as minimal as possible

It looks like some older devices will not work without legacy mode.

what do you mean?

@mrskycriper
Copy link
Contributor Author

what do you mean?

I mean that right now they mostly don't work if legacy mode is not enabled.

Like here:

lumi_buttons: {
    cluster: 'genOnOff',
    type: ['attributeReport'],
    options: [exposes.options.legacy()],
    convert: (model, msg, publish, options, meta) => {
        if (isLegacyEnabled(options)) {
            const mapping: KeyValueAny = {4: 'left', 5: 'right'};
            const button = mapping[msg.endpoint.ID];
            if (button) {
                const payload: KeyValueAny = {};
                payload[`button_${button}`] = msg.data['onOff'] === 1 ? 'release' : 'hold';
                return payload;
            }
        }
    },
} satisfies Fz.Converter

But most importantly, what are the general differences between normal and legacy converters and how to update them?

@Koenkk
Copy link
Owner

Koenkk commented Jan 31, 2024

But most importantly, what are the general differences between normal and legacy converters and how to update them?

These converters were kept to prevent a breaking change, therefore these were moved to legacy. On new installations legacy: false is automatically set in the z2m configuration.yaml, so legacy converters will not be used.

@mrskycriper
Copy link
Contributor Author

These converters were kept to prevent a breaking change, therefore these were moved to legacy. On new installations legacy: false is automatically set in the z2m configuration.yaml, so legacy converters will not be used.

I've merged them all into non legacy ones. Does this look reasonable or they should be kept is separate but consolidated legacy converters?

@Koenkk
Copy link
Owner

Koenkk commented Feb 3, 2024

Let's leave all legacy in legacy.ts, this file will be completely removed in the future. Spend as little time as possible on it.

@mrskycriper mrskycriper marked this pull request as ready for review February 10, 2024 22:11
@mrskycriper
Copy link
Contributor Author

Let's leave all legacy in legacy.ts, this file will be completely removed in the future. Spend as little time as possible on it.

@Koenkk left legacy alone but moved to lib/limi.ts for easier refactoring later.

Moved all Lumi related converters lib/lumi.ts, renamed and combined where possible. Probably not ideal and some more generalization is possible, but I feel like this is enough.

Merged a lot of conflicts, hopefully without regressions. Let me know if I can help with code review in some way as it's and enormous diff.

@Koenkk Koenkk merged commit f666002 into Koenkk:master Feb 11, 2024
2 checks passed
@Koenkk
Copy link
Owner

Koenkk commented Feb 11, 2024

ThankS!

@mrskycriper mrskycriper deleted the lumi-refactoring branch February 12, 2024 00:10
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.

None yet

2 participants