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

add tompd_63wifi_breaker #2618

Merged
merged 3 commits into from
Dec 8, 2024
Merged

Conversation

AlexK98
Copy link
Contributor

@AlexK98 AlexK98 commented Dec 6, 2024

Add configuration for Tomzn TOMPD-63WIFI circuit breaker.

Add configuration for Tomzn TOMPD-63WIFI circuit breaker.
fix presumably wrong placement of MASK parameter
@make-all
Copy link
Owner

make-all commented Dec 8, 2024

This seems very similar to existing TOMP 63 config.
Differences:

  • Refresh sensors button (dp 106) was removed. As this is already marked optional, it should not interfere with detecting your device, and if not functional, you can hide it.
  • Earth leak test button (dp 21) was removed. As above, it is already marked optional and can just be ignored if it is not working.
  • config options for dp 104 and 105 were added. As above, we could just add these as optional to the existing config.
  • some renaming, which I agree with except the names should have been removed completely to let class translations be used.
  • removal of diagnostic category for some sensors. Since these are not the primary sensors for the device, I think diagnostic is appropriate. The only affect this has is that they are not automatically added to generated dashboards, or voice assistants, but they can be manually added if you want them there.
  • removal of one optional marking on a dp. This doesn't have a major effect, except filtering the config out from some false matches perhaps.

My preference for this device is to modify the existing config with the two new entities you added rather than accept a new config for it.

@make-all make-all closed this Dec 8, 2024
make-all added a commit that referenced this pull request Dec 8, 2024
@make-all make-all reopened this Dec 8, 2024
@make-all
Copy link
Owner

make-all commented Dec 8, 2024

Seeing the conflicts in dps 104 and 105 between this and the tompd 63lw v2 config you also submitted, I'm reconsidering which should be merged with the original tompd 63lw config. Since the name matches, I think it is probably better for the other one to merge (it also only really has additional dps which can be optional), and this one to be new. I will need to clean up the main branch now to rebase this PR so it can be merge.

make-all added a commit that referenced this pull request Dec 8, 2024
Two "new" configs were submitted at the same time:
TOMPD-63-WIFI, and another variant of TOMPD-63LW

Initial analysis showed that the TOMPD-63-WIFI was closer to the original
TOMPD-63LW config, but actually both are just extending it with a few new
entities that can be made optional, and the -WIFI one actually removed
some entities that the 63LW didn't.

Both added numeric dps 104 and 105, but the WIFI has them as config options
for Over/Under voltage handling timeouts, while the 63LW has them as sensors
(power factor and frequency).

Since the 63LW is the same name, it should probably get priority.

PR #2619, reverting previous config merge of PR #2618
- top level name should be generic
- remove names which match class, so translations are used
- restore diagnostic on secondary sensors

PR make-all#2618
@make-all make-all merged commit 6320fc4 into make-all:main Dec 8, 2024
4 checks passed
make-all added a commit that referenced this pull request Dec 8, 2024
@AlexK98 AlexK98 deleted the tomzn-tompd-63wifi branch December 10, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants