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

feat(ignore): Refactor more to modernExtend #7168

Merged
merged 1 commit into from
Mar 6, 2024
Merged

Conversation

Koenkk
Copy link
Owner

@Koenkk Koenkk commented Mar 6, 2024

No description provided.

Copy link
Contributor

github-actions bot commented Mar 6, 2024

Since you are the file owner, could you check this @martyn-vesternet?


Caused by:

@Koenkk Koenkk merged commit ed7ed68 into master Mar 6, 2024
3 checks passed
@Koenkk Koenkk deleted the feat/modern-extend branch March 6, 2024 20:47
@martyn-vesternet
Copy link
Contributor

@Koenkk generally speaking I'm not averse to any changes as such, however have they actually been validated / tested against the Vesternet devices?

I've not looked at the code in depth, but typically when I submit for the Vesternet devices my code has been tested against as many revisions of the devices that I have to hand.

For example this particular change seems to remove the explicit exposes definitions and reporting configuration of the dimmer module and replace it with something generic. Do these generic changes work the same way that my original code did with this device?

I'm not saying there's anything wrong with any of the code changes, but it's hard to keep track of how devices performed during testing that may have occurred months or even years ago. There are currently around 25 Vesternet devices (Zigbee and Z-Wave) across 4 platforms (hubitat, smartthings, homey & home assistant) and I try to ensure that configuration & behaviour is identical across all of them.

There's an outstanding PR at #7020 for example that I haven't gone back to yet for a similar reason.

From a commercial perspective it can cause issues if customers subsequently report problems with their devices caused by code that has not been tested by Vesternet.

Unfortunately I don't have (paid) time to retest every single code change that comes through myself to be sure that nothing breaks.

What do you think is the best way forwards?

Would it be better to revert to just providing code via the Vesternet Github like I do for other platforms - https://github.com/vesternet

@Koenkk
Copy link
Owner Author

Koenkk commented Mar 8, 2024

@martyn-vesternet this was just a general refactor, functionality is not impacted and should be the same.

@martyn-vesternet
Copy link
Contributor

OK, but looking at the code changes, all the exposes and configuration for the VES-ZB-DIM-004 device have been removed.

Instead it seems to now inherit from modernExtend, with different exposes and configuration.

The changes in this commit don't seem "the same" to me. The configuration is definitely different. Will the functionality still use "e.light_brightness().withLevelConfig(['on_transition_time', 'off_transition_time']" for example, I couldn't see that method at all in modernExtend?

Similarly for that other PR with the new Zigbee sensor devices, they were tested with that specific configuration and setup. The suggested changes swapped that configuration with some default from deeper within the codebase, which isn't how the devices were tested.

I've not looked in depth at the code, I have no real desire to get deep into this codebase, I merely want to submit enough code to get these devices working aka a "customer converter", similar to how I do with other platforms.

Essentially what I want is to be able to continue to develop via custom converters ( https://github.com/vesternet/zigbee2mqtt-herdsman-converters/commits/main/ ) and have the same code present here so that I can be sure that customers are using the same code that the devices were developed with.

Koenkk pushed a commit that referenced this pull request Mar 11, 2024
@Koenkk
Copy link
Owner Author

Koenkk commented Mar 11, 2024

I missed the level config indeed, added it back! Btw I believe with the old converter level config was not working as it was missing tz.level_config, that's fixed now.

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.

2 participants