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

fix(ignore): move ubisys clusters/attributes out of zh #7451

Merged
merged 6 commits into from
May 2, 2024

Conversation

sjorge
Copy link
Sponsor Contributor

@sjorge sjorge commented Apr 27, 2024

Depends on modernExtend added in #7450, rebase and merge after that one.

Also depends on #7463, this PR needs a rebase merge.

After this, merge Koenkk/zigbee-herdsman#1034

@sjorge

This comment was marked as resolved.

@sjorge
Copy link
Sponsor Contributor Author

sjorge commented Apr 27, 2024

I don't understand the test failure, I don't think I've changed that code ?
It also of course prevent me from actually testing the whole PR.

Edit this is also used in more locations, which does not seem to trip the assert???

src/devices/bitron.ts:        exposes: (device, options) => {
src/devices/bosch.ts:        exposes: (device, options) => {
src/devices/busch_jaeger.ts:        exposes: (device, options) => {
src/devices/custom_devices_diy.ts:        exposes: (device, options) => {
src/devices/danfoss.ts:        exposes: (device, options) => {
src/devices/develco.ts:        exposes: (device, options) => {
src/devices/imhotepcreation.ts:        exposes: (device, options) => {
src/devices/legrand.ts:        exposes: (device, options) => {
src/devices/legrand.ts:        exposes: (device, options) => {
src/devices/lixee.ts:        exposes: (device, options) => {
src/devices/ls.ts:        exposes: (device, options) => {
src/devices/moes.ts:        exposes: (device, options) => {
src/devices/moes.ts:        exposes: (device, options) => {
src/devices/profalux.ts:        exposes: (device, options) => {
src/devices/siglis.ts:        exposes: (device, options) => {
src/devices/siglis.ts:        exposes: (device, options) => {
src/devices/tuya.ts:        exposes: (device, options) => {
src/devices/tuya.ts:        exposes: (device, options) => {
src/devices/tuya.ts:        exposes: (device, options) => {
src/devices/tuya.ts:        exposes: (device, options) => {
src/devices/ubisys.ts:        exposes: (device, options) => {

Edit2: It seems this is triggered because J1 now has an 'extend: []' block, which it did not have before.
@Koenkk I think the fact that exposes is a function still makes sense here. It changes what is exposed based on the devices configuration which is highly variable.

@sjorge sjorge marked this pull request as draft April 27, 2024 11:25
src/lib/ubisys.ts Outdated Show resolved Hide resolved
@sjorge
Copy link
Sponsor Contributor Author

sjorge commented Apr 28, 2024

I'll rebase after #7450 lands.

@sjorge sjorge force-pushed the ubisys_clusters branch 3 times, most recently from ca8abc3 to 493520f Compare April 28, 2024 17:41
src/lib/ubisys.ts Outdated Show resolved Hide resolved
@sjorge
Copy link
Sponsor Contributor Author

sjorge commented Apr 28, 2024

All seems good, I tested both TRV vacationMode changes, S1+C4+S2 input_action configuration and a full J1 calibration

[2024-04-28 22:05:01] warning:  zhc:ubisys: ubisys:   Done - will now read back the results.
[2024-04-28 22:05:01] warning:  zhc:ubisys: ubisys: Cover configuration read: {"windowCoveringType":0,"physicalClosedLimitLiftCm":65535,"physicalClosedLimitTiltDdegree":65535,"installedOpenLimitLiftCm":0,"installedClosedLimitLiftCm":240,"installedOpenLimitTiltDdegree":0,"installedClosedLimitTiltDdegree":900}

All seems to still work, but the J1 calibration is the most complex so I focused mostly on that.

I tested this with both ZH's cluster definition unchaged and with the ubisys entries removed in ZH.

@burmistrzak
Copy link
Contributor

@sjorge So the idea here is for ZH to eventually only include what's actually in the ZCL spec, right?
What do you think, does it still make sense to add new Bosch-specific definitions to ZH, or should I wait until I can also refactor bosch.ts in a similar fashion?

@sjorge
Copy link
Sponsor Contributor Author

sjorge commented May 2, 2024

@sjorge So the idea here is for ZH to eventually only include what's actually in the ZCL spec, right? What do you think, does it still make sense to add new Bosch-specific definitions to ZH, or should I wait until I can also refactor bosch.ts in a similar fashion?

You can probably start by adding the new ones in zhc on the devices that need them, then moving the other ones out of zh into zhc can come after. It's fine to have some in zhc and some still in zh, so new ones don't get added to zh IMHO.

Once @Koenkk merges #7463 I will rebase this PR and this one should be ready for merging too.

@Koenkk
Copy link
Owner

Koenkk commented May 2, 2024

Merged #7463 !

@sjorge sjorge force-pushed the ubisys_clusters branch 4 times, most recently from cec56c1 to 0412c73 Compare May 2, 2024 19:36
@sjorge sjorge marked this pull request as ready for review May 2, 2024 19:37
@sjorge
Copy link
Sponsor Contributor Author

sjorge commented May 2, 2024

I only give it another quick retest after changing the casing of the DataType, I just turned on/off some of the switches. I did not do a full J1 reconfigure as it's rather noisy to make the blinds go up and down a few times during calibration and it's already pretty late.

@burmistrzak
Copy link
Contributor

You can probably start by adding the new ones in zhc on the devices that need them, then moving the other ones out of zh into zhc can come after. It's fine to have some in zhc and some still in zh, so new ones don't get added to zh IMHO.

Yea, sounds good. 👌
Will eventually give it a try with something low-risk. 😅

@Koenkk Koenkk merged commit 5a02438 into Koenkk:master May 2, 2024
2 checks passed
@Koenkk
Copy link
Owner

Koenkk commented May 2, 2024

Thanks!

@sjorge sjorge deleted the ubisys_clusters branch May 2, 2024 20:14
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

3 participants