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

Update oregon_scientific.c #2086

Merged
merged 1 commit into from
Jan 22, 2023
Merged

Update oregon_scientific.c #2086

merged 1 commit into from
Jan 22, 2023

Conversation

elljay23
Copy link
Contributor

@elljay23 elljay23 commented Jun 8, 2022

Hi, I've added some sanity checks to confirm BCD digits are only 0-9 and check sensor values aren't outside their maximum range. I've also corrected the flag to set the negative sign on temperature values. It was incorrectly including the bit that denotes "maximum range limit reached". I've been running the updates against my sensors for some weeks and they've stopped the screwy values I was seeing from rtl_433 every day or so.

This is my first pull request so I hope I've got the process right! :)

Thanks, Laurence

Add sanity checks
return temp_c;
}

static float get_os_rain_rate(unsigned char *message)
{
float rain_rate = 0; // Nibbles 11..8 rain rate, LSD = 0.01 inches per hour
rain_rate = (((message[5] & 0x0f) * 1000) + ((message[5] >> 4) * 100) + ((message[4] & 0x0f) * 10) + ((message[4] >> 4) & 0x0f)) / 100.0F;
float rain_rate = 0.0F; // Nibbles 11..8 rain rate, LSD = 0.1 units per hour, 4321 = 123.4 units per hour
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is later used as "rain_rate_in_h", can you confirm that units is in/h here? Or Is a unit 0.1 in, thus the change of scale wrong here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, good question. Not sure why I edited that section... The sensor generated rain_rate lags so much I don't use it myself (I calculate it based on when the rain_total field changes.) I used the page here: https://www.bashewa.com/wmr200-protocol.php as a starting point and compared it with the data I get from a PCR800 sensor. I believe the PCR800 reports it in units of 0.1 inches per hour.

total_rain = (message[8] & 0x0f) * 100.0F
+ ((message[8] >> 4) & 0x0f) * 10.0F + (message[7] & 0x0f)
+ ((message[8] >> 4) & 0x0f) * 10.0F + (message[7] & 0x0f)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Trailing whitespace

@@ -868,7 +922,7 @@ static char *output_fields[] = {
r_device oregon_scientific = {
.name = "Oregon Scientific Weather Sensor",
.modulation = OOK_PULSE_MANCHESTER_ZEROBIT,
.short_width = 440, // Nominal 1024Hz (488µs), but pulses are shorter than pauses
.short_width = 440, // Nominal 1024Hz (488µs), but pulses are shorter than pauses
Copy link
Collaborator

Choose a reason for hiding this comment

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

UTF8/Latin1 conversion problems introduced an invalid code here. Best to replace µs with us

@zuckschwerdt
Copy link
Collaborator

How did this slip through? A genuine fix for negative temps and needed sanity checks, thanks!

@zuckschwerdt zuckschwerdt added feedback request for more information; may be closed id 30d if not received and removed feedback request for more information; may be closed id 30d if not received labels Jan 21, 2023
@elljay23
Copy link
Contributor Author

elljay23 commented Jan 21, 2023

How did this slip through? A genuine fix for negative temps and needed sanity checks, thanks!

Welcome :) I've been running this code 24 hours a day since June and I no longer see any issues with "BCD" values outside 0 to 9 being converted to decimal and producing screwy jumps in the data.

@zuckschwerdt
Copy link
Collaborator

zuckschwerdt commented Jan 21, 2023

Can you fix those 3 places (revert the "units" to inches per hour, remove the trailing space, change the mangled "µ" char to "u") and just add another commit on top. Good to merge then!

@zuckschwerdt
Copy link
Collaborator

Merging with a force push to correct the above 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