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 Support for Ecowitt WN34D and improve FineOffset WN34 #2944

Merged
merged 2 commits into from
May 30, 2024

Conversation

ProfBoc75
Copy link
Collaborator

Related to issue #2943

Update the existing decoder to take into account this model WN34D, temperature without offset.
Few document updates and improvement.

@ProfBoc75 ProfBoc75 linked an issue May 28, 2024 that may be closed by this pull request
@zuckschwerdt
Copy link
Collaborator

I think subtype might be a good choice here as it is a different "function" (coding of the value) in the same protocol.
But this will be a breaking change as the subtype is part of the MQTT key.

As a rare exception I would rather change the "model" key here to not break existing usage, i.e.

"model", "", DATA_COND, sub_type != 4, DATA_STRING, "Fineoffset-WN34",
"model", "", DATA_COND, sub_type == 4, DATA_STRING, "Fineoffset-WN34D",

and don't report subtype.

What do you think?

@ProfBoc75
Copy link
Collaborator Author

I think subtype might be a good choice here as it is a different "function" (coding of the value) in the same protocol. But this will be a breaking change as the subtype is part of the MQTT key.

As a rare exception I would rather change the "model" key here to not break existing usage, i.e.

"model", "", DATA_COND, sub_type != 4, DATA_STRING, "Fineoffset-WN34",
"model", "", DATA_COND, sub_type == 4, DATA_STRING, "Fineoffset-WN34D",

and don't report subtype.

What do you think?

Yes, good approach, I'm updating the decoder. Instead, I will add the subtype as a verbose message.
Because we know the sub-type is 0x4 for WN34D, and it's 0x0 for WN34L ( cu8 files from this PR at rtl_433_tests), but we don't know for other models, may be other values than 0 & 4, I would like to catch, the reason why I'm asking for some feedback in the issue.

In the same time, I was also wondering how to show this model, because the decoder is always reporting WN34, and yet it's able to decode other models WN34L/S and D, your approach answers to this concern, and I will enrich also the name accordingly.

Good point, thx.

@Mart124
Copy link

Mart124 commented May 30, 2024

Many thanks @ProfBoc75 for your work and @zuckschwerdt for your review 👍
Could we think about merging this patch if everything is OK with it ? So that we can begin using it :)
Thank you again !

@zuckschwerdt
Copy link
Collaborator

Guess it doesn't matter too much, we can further enlarge the allowed length to 500. Do you want to PR the change?

@ProfBoc75 ProfBoc75 merged commit ec72c8c into master May 30, 2024
16 checks passed
@ProfBoc75 ProfBoc75 deleted the feat-ecowitt-wn34d branch May 31, 2024 18:58
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 Ecowitt water sensor, PN WN34D
3 participants