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 the TFA 30.3221.20 temperature/humidity sensor #471

Open
wants to merge 9 commits into
base: staging
Choose a base branch
from

Conversation

jtheuer
Copy link

@jtheuer jtheuer commented Feb 20, 2023

This PR adds support for the TFA Dostmann 30.3221.20 temperature/humidity sensor.

It is based on the ground work of github.com/NorthernMan54/rtl_433_ESP/blob/master/contrib/lacrosse_tx141x.c and partly inspired by github.com/RFD-FHEM/RFFHEM/blob/master/FHEM/14_SD_WS.pm.

This code runs in "production" for several weeks now. It is however only used on an ESP32 via github.com/puuu/ESPiLight. (Related PR: puuu/ESPiLight#67).

Caveat: It only implements the methods that could be used and tested on the ESP32 (namely the temperature/humidity parsing).

Todo:

  • waiting for feedback on the draft.

if (headerFound == HEADER_PULSE_COUNT) {
return i; // this is the first pulse/gap pair that didn't look like a header
} else {
return -headerFound; // header not yet complete - broken transmission
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the need for the headerFound * -1 instead of just a -1 for errors? The same question about the -999

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was useful during testing to know how many header packets had been found (hence the -headerFound as response). The -999 was an (albeit ugly) hack to signal that the transmission is invalid but not messing with "the other -1". I can either document the function or remove it entirely if you have an opinion about it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uses now -1 always

for (int i = 0; i < 40; i++) { // read 40 value-pairs from raw data
int v0 = tfa303221->raw[payloadStartIndex + (i * 2)];
int v1 = tfa303221->raw[payloadStartIndex + (i * 2) + 1];
if (v0 < 357 && v1 > 357) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this related to the pulse lengths?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, v0/v1 are pulse length pairs (shot-long or long-short).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched to use of a constant for 357

@CurlyMoo
Copy link
Contributor

All in all it seems to be a pretty well make PR. Some questions:

  • Can this be licensed MPLv2?
  • Can you follow the pilight code styling?
  • Can you also do a PR to the rewrite branch including a unittest?
  • Can you add a documentation page for this device?

@jtheuer
Copy link
Author

jtheuer commented Feb 27, 2023

  • Can this be licensed MPLv2?

My code for sure. For the parts I copied I need to ask the original authors. Would that be a blocker otherwise?
I assume I'd need to dual-license it for now (because pilight is still GPL)?

  • Can you follow the pilight code styling?

I went through the style guide again and I think it matches now. Let me know if you spot something missing. That said, if you have a vscode style or a linter to share I'm happy to test it myself.

  • Can you also do a PR to the rewrite branch including a unittest?

I like unit testing. Let me check if I can understand how it works in C.

  • Can you add a documentation page for this device?

Sure, do you have a favourite for me to use as style template? Don't worry if not, I'll just pick one from the other TFA * ones.

@CurlyMoo
Copy link
Contributor

I assume I'd need to dual-license it for now (because pilight is still GPL)?

Yes, because it contains GPL parts. The only way to fix that would be to slowly remove all GPL and at least not accept new GPL stuff.

Sure, do you have a favourite for me to use as style template? Don't worry if not, I'll just pick one from the other TFA * ones.

Just adapt one of the manual pages.

@Andy-Voigt
Copy link

@jtheuer Would be nice if you update your code to integrate this protocol.

@jtheuer jtheuer marked this pull request as ready for review January 13, 2024 17:26
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.

3 participants