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

Acurite 5n1 changes #463

Merged
merged 13 commits into from Jan 13, 2017
1 change: 0 additions & 1 deletion include/rtl_433_devices.h
Expand Up @@ -12,7 +12,6 @@
DECL(elv_em1000) \
DECL(elv_ws2000) \
DECL(lacrossetx) \
DECL(acurite5n1) \
DECL(acurite_rain_gauge) \
Copy link
Owner

Choose a reason for hiding this comment

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

You need to register the template decoder here again. Otherwise you will mess up ordering and people will have to change their scripts.

Copy link
Contributor

@rct rct Jan 7, 2017

Choose a reason for hiding this comment

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

Hmm, I'm not sure what's better behavior:
. silently changing protocol 9 to output the same thing as protocol 39
. "Breaking" protocol 9 to make it a silent NO-OP
. Throw an error if -R 9 is enabled (but not if -G is used.)

@matthewwall might have an opinion for how it affects his WeeWx driver.

Note: If this code gets enabled/run by using either -G or -R 9 -R 39, there is a subtle bug that rain will be counted twice. Though see my next comment for changing the rain state keeping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we have a "special" null device type, e.g deleted, which we can register here as a placeholder not to mess up the index ordering?

I could leave this driver alone (I recall writing much of the initial driver, although it has been modified) in here, but its both duplicative and broken. Moving to a single one if it covers these use cases is probably better - but it's an interesting question as to the right approach.

Copy link
Owner

Choose a reason for hiding this comment

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

DECL(template) \

Register that again. Then the order will be kept.

Choose a reason for hiding this comment

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

@rct - with respect to protocol numbering, weewx-sdr does not care. it parses the rtl_433 output (json or non-json), so protocol numbering does not affect how it behaves.

DECL(acurite_th) \
DECL(oregon_scientific) \
Expand Down
192 changes: 76 additions & 116 deletions src/devices/acurite.c
Expand Up @@ -153,15 +153,18 @@ static float acurite_getTemp (uint8_t highbyte, uint8_t lowbyte) {
return temp;
}

static int acurite_getWindSpeed (uint8_t highbyte, uint8_t lowbyte) {
static float acurite_getWindSpeed_kph (uint8_t highbyte, uint8_t lowbyte) {
// range: 0 to 159 kph
// TODO: sensor does not seem to be in kph, e.g.,
// a value of 49 here was registered as 41 kph on base unit
// value could be rpm, etc which may need (polynomial) scaling factor??
// raw number is cup rotations per 4 seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I believe it is the highest number of rotations that occurred in 4 second buckets since the last wind speed report.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that detailed info in the link from the original source of information

Copy link
Contributor

Choose a reason for hiding this comment

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

(and I wound up testing that was correct when I had to take my 5n1 down this past fall to replace a failed temperature/humidity sensor.

// http://www.wxforum.net/index.php?topic=27244.0 (found from weewx driver)
int highbits = ( highbyte & 0x1F) << 3;
int lowbits = ( lowbyte & 0x70 ) >> 4;
int speed = highbits | lowbits;
return speed;
int rawspeed = highbits | lowbits;
float speed_kph = 0;
if (rawspeed > 0) {
speed_kph = rawspeed * 0.8278 + 1.0;
}
return speed_kph;
}

// For the 5n1 based on a linear/circular encoding.
Expand Down Expand Up @@ -232,64 +235,6 @@ static int acurite_5n1_getBatteryLevel(uint8_t byte){
}


int acurite5n1_callback(bitbuffer_t *bitbuffer) {
// acurite 5n1 weather sensor decoding for rtl_433
// Jens Jensen 2014
bitrow_t *bb = bitbuffer->bb;
int i;
uint8_t *buf = NULL;
// run through rows til we find one with good checksum (brute force)
for (i=0; i < BITBUF_ROWS; i++) {
if (acurite_detect(bb[i])) {
buf = bb[i];
break; // done
}
}

if (buf) {
// decode packet here
if (debug_output) {
fprintf(stdout, "Detected Acurite 5n1 sensor, %d bits\n",bitbuffer->bits_per_row[1]);
for (i=0; i < 8; i++)
fprintf(stdout, "%02X ", buf[i]);
fprintf(stdout, "Checksum OK\n");
}

if ((buf[2] & 0x0F) == 1) {
// wind speed, wind direction, rainfall

float rainfall = 0.00;
int raincounter = acurite_getRainfallCounter(buf[5], buf[6]);
if (acurite_5n1raincounter > 0) {
// track rainfall difference after first run
rainfall = ( raincounter - acurite_5n1raincounter ) * 0.01;
} else {
// capture starting counter
acurite_5n1raincounter = raincounter;
}

fprintf(stdout, "wind speed: %d kph, ",
acurite_getWindSpeed(buf[3], buf[4]));
fprintf(stdout, "wind direction: %0.1f°, ",
acurite_getWindDirection(buf[4]));
fprintf(stdout, "rain gauge: %0.2f in.\n", rainfall);

} else if ((buf[2] & 0x0F) == 8) {
// wind speed, temp, RH
fprintf(stdout, "wind speed: %d kph, ",
acurite_getWindSpeed(buf[3], buf[4]));
fprintf(stdout, "temp: %2.1f° F, ",
acurite_getTemp(buf[4], buf[5]));
fprintf(stdout, "humidity: %d%% RH\n",
acurite_getHumidity(buf[6]));
}
} else {
return 0;
}

return 1;
}

static int acurite_rain_gauge_callback(bitbuffer_t *bitbuffer) {
bitrow_t *bb = bitbuffer->bb;
// This needs more validation to positively identify correct sensor type, but it basically works if message is really from acurite raingauge and it doesn't have any errors
Expand Down Expand Up @@ -405,12 +350,12 @@ static float acurite_txr_getTemp (uint8_t highbyte, uint8_t lowbyte) {
static int acurite_txr_callback(bitbuffer_t *bitbuf) {
int browlen, valid = 0;
uint8_t *bb;
float tempc, tempf, wind_dird, rainfall = 0.0, wind_speedmph;
float tempc, tempf, wind_dird, rainfall = 0.0, wind_speed, wind_speedmph;
uint8_t humidity, sensor_status, repeat_no, message_type;
char channel, *wind_dirstr = "";
char channel_str[2];
uint16_t sensor_id;
int wind_speed, raincounter, temp, battery_low;
int raincounter, temp, battery_low;
uint8_t strike_count, strike_distance;
data_t *data;

Expand Down Expand Up @@ -490,55 +435,80 @@ static int acurite_txr_callback(bitbuffer_t *bitbuf) {

// The 5-n-1 weather sensor messages are 8 bytes.
if (browlen == ACURITE_5N1_BITLEN / 8) {
if (debug_output) {
fprintf(stderr, "Acurite 5n1 raw msg: %02X %02X %02X %02X %02X %02X %02X %02X\n",
bb[0], bb[1], bb[2], bb[3], bb[4], bb[5], bb[6], bb[7]);
}
channel = acurite_getChannel(bb[0]);
sprintf(channel_str, "%c", channel);
sensor_id = acurite_5n1_getSensorId(bb[0],bb[1]);
repeat_no = acurite_5n1_getMessageCaught(bb[0]);
message_type = bb[2] & 0x3f;

battery_low = (bb[2] & 0x40) >> 6;

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a 5n1 battery function that I think is now no longer used https://github.com/merbanan/rtl_433/blob/master/src/devices/acurite.c#L230.

Also, I had in my notes that I wanted to output that status byte, (or the bits of it that didn't appear to be used for anything else) when moving to JSON. The idea being that there might be some other undecoded status bits in there beside the battery low.

if (message_type == 0x31) {
// Wind speed, wind direction, and rain fall
wind_speed = acurite_getWindSpeed(bb[3], bb[4]);
wind_speedmph = kmph2mph(wind_speed);
wind_dird = acurite_5n1_winddirections[bb[4] & 0x0f];
wind_dirstr = acurite_5n1_winddirection_str[bb[4] & 0x0f];
raincounter = acurite_getRainfallCounter(bb[5], bb[6]);
if (acurite_5n1t_raincounter > 0) {
// track rainfall difference after first run
// FIXME when converting to structured output, just output
// the reading, let consumer track state/wrap around, etc.
rainfall = ( raincounter - acurite_5n1t_raincounter ) * 0.01;
if (raincounter < acurite_5n1t_raincounter) {
printf("%s Acurite 5n1 sensor 0x%04X Ch %c, rain counter reset or wrapped around (old %d, new %d)\n",
time_str, sensor_id, channel, acurite_5n1t_raincounter, raincounter);
acurite_5n1t_raincounter = raincounter;
}
} else {
// capture starting counter
acurite_5n1t_raincounter = raincounter;
printf("%s Acurite 5n1 sensor 0x%04X Ch %c, Total rain fall since last reset: %0.2f\n",
time_str, sensor_id, channel, raincounter * 0.01);
}

printf("%s Acurite 5n1 sensor 0x%04X Ch %c, Msg %02x, Wind %d kmph / %0.1f mph %0.1f° %s (%d), rain gauge %0.2f in.\n",
time_str, sensor_id, channel, message_type,
wind_speed, wind_speedmph,
wind_dird, wind_dirstr, bb[4] & 0x0f, rainfall);
// Wind speed, wind direction, and rain fall
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the indentation level changed, should it have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, I think acurite.c suffers from a mix of tab vs spaces indentation. I can do a separate reformatting cleanup commit later - didn't want to completely pollute it here ;)

wind_speed = acurite_getWindSpeed_kph(bb[3], bb[4]);
wind_speedmph = kmph2mph(wind_speed);
wind_dird = acurite_5n1_winddirections[bb[4] & 0x0f];
wind_dirstr = acurite_5n1_winddirection_str[bb[4] & 0x0f];
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, the wind directions for this are different than the old protocol 9 decoder. The old decoder used a linear (IIRC) mapping which didn't seem to be correct. The 5n1 seems to use a modified gray code. I've made sure that the newer (protocol 39) decoder makes what the aculink internet bridge reports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we are talking abotu this one?
https://github.com/merbanan/rtl_433/blob/master/src/devices/acurite.c#L66-L70
If I'm cleaning up the code, I'll go ahead and pull those orphaned stubs out as well.

I believe I came up with that original list (or in cohort with Helge). I suspect that it is wrong. I just ran through my 5n1 (vn1tx), and protocol 39 is correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. There are/will be a few more unused variables and functions once the old code is removed. Don't know if the compiler will catch them.

raincounter = acurite_getRainfallCounter(bb[5], bb[6]);
if (acurite_5n1t_raincounter > 0) {
// track rainfall difference after first run
// FIXME when converting to structured output, just output
// the reading, let consumer track state/wrap around, etc.
rainfall = ( raincounter - acurite_5n1t_raincounter ) * 0.01;
Copy link
Contributor

@rct rct Jan 7, 2017

Choose a reason for hiding this comment

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

As per this comment, I really think the decoder should just output the raw rain counter value (* 0.01) and not try to keep state. I think now that the code is being changed to JSON/KV/CSV output is good time to make this non-upward compatible change. The downstream clients should be the ones that keep state, notice if a reset/wrap around occurs, and determine what to do if you are receiving more than 1 5-n-1 sensor.

@matthewwall do you have an opinion 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.

What about outputting both the accumulated value (as recorded since the start of rtl_433) and the raw value (i.e. number of bucket tips, e.g. for external processing) - this should be easy enough.

Note that I have already discovered a big bug in the rain accumulation code - if there is more than one 5n1 (vn1tx) rtl_433 will get confused because it's not treating these as separate instances and constantly flip-flopping between them, example:

acurite_txr Parity: 11000000
{"time" : "2017-01-07 12:18:00", "model" : "Acurite 5n1 sensor", "sensor_id" : 3585, "channel" : "B", "battery" : "LOW", "message_type" : 56, "wind_speed" : 0.000, "temperature_C" : 18.778, "humidity" : 29}
acurite_txr Parity: 00000000
2017-01-07 12:18:15 Acurite 5n1 sensor 0x0688 Ch A, Total rain fall since last reset: 103.15
{"time" : "2017-01-07 12:18:15", "model" : "Acurite 5n1 sensor", "sensor_id" : 1672, "channel" : "A", "battery" : "LOW", "message_type" : 49, "wind_speed" : 5.139, "wind_dir_deg" : 22.500, "wind_dir" : "NNE", "rainfall" : 0.000}
acurite_txr Parity: 10000000
{"time" : "2017-01-07 12:18:15", "model" : "Acurite 5n1 sensor", "sensor_id" : 1672, "channel" : "A", "battery" : "LOW", "message_type" : 49, "wind_speed" : 5.139, "wind_dir_deg" : 22.500, "wind_dir" : "NNE", "rainfall" : 0.000}
acurite_txr Parity: 10000001
{"time" : "2017-01-07 12:18:15", "model" : "Acurite 5n1 sensor", "sensor_id" : 1672, "channel" : "A", "battery" : "LOW", "message_type" : 49, "wind_speed" : 5.139, "wind_dir_deg" : 22.500, "wind_dir" : "NNE", "rainfall" : 0.000}
acurite_txr Parity: 01000001
2017-01-07 12:18:19 Acurite 5n1 sensor 0x0E01 Ch B, rain counter reset or wrapped around (old 10315, new 0)
{"time" : "2017-01-07 12:18:19", "model" : "Acurite 5n1 sensor", "sensor_id" : 3585, "channel" : "B", "battery" : "LOW", "message_type" : 49, "wind_speed" : 0.000, "wind_dir_deg" : 0.000, "wind_dir" : "N", "rainfall" : -103.150}
acurite_txr Parity: 11000000
2017-01-07 12:18:19 Acurite 5n1 sensor 0x0E01 Ch B, Total rain fall since last reset: 0.00
{"time" : "2017-01-07 12:18:19", "model" : "Acurite 5n1 sensor", "sensor_id" : 3585, "channel" : "B", "battery" : "LOW", "message_type" : 49, "wind_speed" : 0.000, "wind_dir_deg" : 0.000, "wind_dir" : "N", "rainfall" : -103.150}
acurite_txr Parity: 11000000
2017-01-07 12:18:19 Acurite 5n1 sensor 0x0E01 Ch B, Total rain fall since last reset: 0.00
{"time" : "2017-01-07 12:18:19", "model" : "Acurite 5n1 sensor", "sensor_id" : 3585, "channel" : "B", "battery" : "LOW", "message_type" : 49, "wind_speed" : 0.000, "wind_dir_deg" : 0.000, "wind_dir" : "N", "rainfall" : -103.150}

Choose a reason for hiding this comment

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

most weather station hardware sends an accumulated value rather than a delta. for some hardware, the accumulate resets each day, for others it resets when the hardware counter wraps around.

generally speaking, an accumulated value is less error-prone that deltas; if you are sending deltas and you lose a packet, you've lost data, if you're sending deltas and you send a packet twice, you have double-counted.

on the other hand, it sucks to maintain state.

in this case, i would recommend that the rtl_433 process maintain the accumulated state of the rain counter. restart rtl_433, and the counter starts again.

any downstream consumer will have no problem, and treat it as a counter wraparound.

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly, I've been anticipating that and surprised no one has complained about that before.

To accurately keep state, you've got to keep track of which 5n1 you are decoding. Also how long should you keep state for? It only really makes sense when you've restarted rtl_433 recently or at an interval that makes sense to you, i.e. start of day, week, month, etc.

As long as the raw value is there

Choose a reason for hiding this comment

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

when acurite updated the firmware, they changed the behavior of the bridge. the old firmware (126) used to send a rain total that would only reset when the 16-bit integer (i think) wrapped around. the new firmware (224) resets the rain counter at midnight each day. in fact, acurite is having problems getting this new approach to work properly - many bridges seem to be spuriously resetting their counters.

Choose a reason for hiding this comment

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

sorry, i'm confusing output from the acuritebridge/smarthub versus output from the 5in1 sensor cluster.

if possible, rtl_433 should send the raw counter, not the deltas. hopefully that will not require maintaining any state in rtl_433.

if (raincounter < acurite_5n1t_raincounter) {
fprintf(stderr, "%s Acurite 5n1 sensor 0x%04X Ch %c, rain counter reset or wrapped around (old %d, new %d)\n",
time_str, sensor_id, channel, acurite_5n1t_raincounter, raincounter);
acurite_5n1t_raincounter = raincounter;
}
} else {
// capture starting counter
acurite_5n1t_raincounter = raincounter;
fprintf(stderr, "%s Acurite 5n1 sensor 0x%04X Ch %c, Total rain fall since last reset: %0.2f\n",
Copy link
Owner

Choose a reason for hiding this comment

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

Add the fprintfs under debug guards.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's information, not a debug message. It is notification that the counter has either reset or wrapped around.

It becomes a moot point if rtl_433 stops trying to keep rain counter state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the original output before i modified it, just moved from a printf to a fprint to stderr, such that it doesn't interfere with formatted output, e.g. json. I didn't want to mess with the rain counter logic just yet because it has a few bugs. However, I guess I can convert this to a separate data message.

time_str, sensor_id, channel, raincounter * 0.01);
}

data = data_make(
"time", "", DATA_STRING, time_str,
"model", "", DATA_STRING, "Acurite 5n1 sensor",
"sensor_id", NULL, DATA_FORMAT, "0x%02X", DATA_INT, sensor_id,
"channel", NULL, DATA_STRING, &channel_str,
"battery", NULL, DATA_STRING, battery_low ? "OK" : "LOW",
"message_type", NULL, DATA_INT, message_type,
"wind_speed", NULL, DATA_FORMAT, "%.1f km/h", DATA_DOUBLE, wind_speed,
"wind_dir_deg", NULL, DATA_FORMAT, "%.1f", DATA_DOUBLE, wind_dird,
"wind_dir", NULL, DATA_STRING, wind_dirstr,
"rainfall", NULL, DATA_FORMAT, "%.2f in", DATA_DOUBLE, rainfall,
NULL);

data_acquired_handler(data);

} else if (message_type == 0x38) {
Copy link
Owner

Choose a reason for hiding this comment

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

If you feel like it you can define types for the message_type numbers. That will make it easier to understand the code later.

// Wind speed, temperature and humidity
wind_speed = acurite_getWindSpeed(bb[3], bb[4]);
wind_speedmph = kmph2mph((float) wind_speed);
tempf = acurite_getTemp(bb[4], bb[5]);
tempc = fahrenheit2celsius(tempf);
humidity = acurite_getHumidity(bb[6]);

printf("%s Acurite 5n1 sensor 0x%04X Ch %c, Msg %02x, Wind %d kmph / %0.1f mph, %3.1F C %3.1F F %d %% RH\n",
time_str, sensor_id, channel, message_type,
wind_speed, wind_speedmph, tempc, tempf, humidity);
// Wind speed, temperature and humidity
Copy link
Contributor

Choose a reason for hiding this comment

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

again, did indentation need to change here?

wind_speed = acurite_getWindSpeed_kph(bb[3], bb[4]);
wind_speedmph = kmph2mph(wind_speed);
tempf = acurite_getTemp(bb[4], bb[5]);
tempc = fahrenheit2celsius(tempf);
humidity = acurite_getHumidity(bb[6]);

data = data_make(
"time", "", DATA_STRING, time_str,
"model", "", DATA_STRING, "Acurite 5n1 sensor",
"sensor_id", NULL, DATA_FORMAT, "0x%02X", DATA_INT, sensor_id,
"channel", NULL, DATA_STRING, &channel_str,
"battery", NULL, DATA_STRING, battery_low ? "OK" : "LOW",
"message_type", NULL, DATA_INT, message_type,
"wind_speed", NULL, DATA_FORMAT, "%.1f km/h", DATA_DOUBLE, wind_speed,
"temperature_C", "temperature", DATA_FORMAT, "%.1f C", DATA_DOUBLE, tempc,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be reporting Fahrenheit, since that appears to be the native unit of the 5n1. The 5n1 data format looks to be Fahrenheit to a 1/10th of a degree. The problem with this, is to get back to Fahrenheit, you've got to double convert F->C, C->F, and wind up with the wrong answer because the float has been pretty printed down to a signal decimal place

"humidity", NULL, DATA_FORMAT, "%d", DATA_INT, humidity,
NULL);
data_acquired_handler(data);

} else {
printf("%s Acurite 5n1 sensor 0x%04X Ch %c, Status %02X, Unknown message type 0x%02x\n",
time_str, sensor_id, channel, bb[3], message_type);
fprintf(stderr, "%s Acurite 5n1 sensor 0x%04X Ch %c, Status %02X, Unknown message type 0x%02x\n",
time_str, sensor_id, channel, bb[3], message_type);
}
}

Expand Down Expand Up @@ -800,16 +770,6 @@ static int acurite_606_callback(bitbuffer_t *bitbuf) {
return 0;
}

r_device acurite5n1 = {
.name = "Acurite 5n1 Weather Station",
.modulation = OOK_PULSE_PWM_RAW,
.short_limit = 280,
.long_limit = 520,
.reset_limit = 800,
.json_callback = &acurite5n1_callback,
.disabled = 1,
.demod_arg = 0,
};

r_device acurite_rain_gauge = {
.name = "Acurite 896 Rain Gauge",
Expand Down