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

Conversation

Projects
None yet
5 participants
@zerog2k
Contributor

zerog2k commented Jan 7, 2017

  • cleanup (remove) acurite5n1, no longer needed as acurite_txr decodes this
  • move acurite_txr::5n1 output to new internal format (to allow output formatting, e.g. in json)
  • output battery status
  • correct wind speed formula
@@ -12,7 +12,6 @@
DECL(elv_em1000) \
DECL(elv_ws2000) \
DECL(lacrossetx) \
DECL(acurite5n1) \

This comment has been minimized.

@merbanan

merbanan Jan 7, 2017

Owner

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

@merbanan

merbanan Jan 7, 2017

Owner

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

This comment has been minimized.

@rct

rct Jan 7, 2017

Contributor

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.

@rct

rct Jan 7, 2017

Contributor

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.

This comment has been minimized.

@zerog2k

zerog2k Jan 7, 2017

Contributor

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.

@zerog2k

zerog2k Jan 7, 2017

Contributor

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.

This comment has been minimized.

@merbanan

merbanan Jan 7, 2017

Owner

DECL(template) \

Register that again. Then the order will be kept.

@merbanan

merbanan Jan 7, 2017

Owner

DECL(template) \

Register that again. Then the order will be kept.

This comment has been minimized.

@matthewwall

matthewwall Jan 12, 2017

@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.

@matthewwall

matthewwall Jan 12, 2017

@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.

} 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",

This comment has been minimized.

@merbanan

merbanan Jan 7, 2017

Owner

Add the fprintfs under debug guards.

@merbanan

merbanan Jan 7, 2017

Owner

Add the fprintfs under debug guards.

This comment has been minimized.

@rct

rct Jan 7, 2017

Contributor

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.

@rct

rct Jan 7, 2017

Contributor

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.

This comment has been minimized.

@zerog2k

zerog2k Jan 7, 2017

Contributor

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.

@zerog2k

zerog2k Jan 7, 2017

Contributor

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.

Show outdated Hide outdated src/devices/acurite.c Outdated
@rct

Summary:
Don't change protocol numbering
Rain gauge implementation should change so rtl_433 doesn't try to keep state about the rain counter to avoid subtle bugs that are hidden from down stream clients.

@@ -12,7 +12,6 @@
DECL(elv_em1000) \
DECL(elv_ws2000) \
DECL(lacrossetx) \
DECL(acurite5n1) \

This comment has been minimized.

@rct

rct Jan 7, 2017

Contributor

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.

@rct

rct Jan 7, 2017

Contributor

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.

// 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

This comment has been minimized.

@rct

rct Jan 7, 2017

Contributor

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

@rct

rct Jan 7, 2017

Contributor

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

This comment has been minimized.

@zerog2k

zerog2k Jan 7, 2017

Contributor

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

@zerog2k

zerog2k Jan 7, 2017

Contributor

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

This comment has been minimized.

@rct

rct Jan 8, 2017

Contributor

(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.

@rct

rct Jan 8, 2017

Contributor

(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.

Show outdated Hide outdated src/devices/acurite.c Outdated
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];

This comment has been minimized.

@rct

rct Jan 7, 2017

Contributor

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.

@rct

rct Jan 7, 2017

Contributor

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.

This comment has been minimized.

@zerog2k

zerog2k Jan 7, 2017

Contributor

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.

@zerog2k

zerog2k Jan 7, 2017

Contributor

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.

This comment has been minimized.

@rct

rct Jan 8, 2017

Contributor

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.

@rct

rct Jan 8, 2017

Contributor

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.

// 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;

This comment has been minimized.

@rct

rct Jan 7, 2017

Contributor

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?

@rct

rct Jan 7, 2017

Contributor

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?

This comment has been minimized.

@zerog2k

zerog2k Jan 7, 2017

Contributor

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}
@zerog2k

zerog2k Jan 7, 2017

Contributor

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}

This comment has been minimized.

@matthewwall

matthewwall Jan 7, 2017

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.

@matthewwall

matthewwall Jan 7, 2017

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.

This comment has been minimized.

@rct

rct Jan 7, 2017

Contributor

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

@rct

rct Jan 7, 2017

Contributor

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

This comment has been minimized.

@matthewwall

matthewwall Jan 7, 2017

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.

@matthewwall

matthewwall Jan 7, 2017

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.

This comment has been minimized.

@matthewwall

matthewwall Jan 7, 2017

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.

@matthewwall

matthewwall Jan 7, 2017

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.

} 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",

This comment has been minimized.

@rct

rct Jan 7, 2017

Contributor

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.

@rct

rct Jan 7, 2017

Contributor

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.

Show outdated Hide outdated src/devices/acurite.c Outdated
@zerog2k

This comment has been minimized.

Show comment
Hide comment
@zerog2k

zerog2k Jan 7, 2017

Contributor

Ok a few updates:

  • register dummy template in place of old removed acurite5n1 driver to prevent device reordering
  • removed references to old driver (specifically, old, probably incorrect wind direction mapping). (I have verified new driver, acurite_txr, is correct with my original vn1tx 5n1 station.)
  • move message types to defines
  • output raw rain counter value in addition to accumulated value (I'll open a separate issue for the one bug I did find with this, which is wrong accumulation with more than one station reporting - however this might turn out to be quite complex - it would probably need state tracking per-sensor id to prevent collision. I feel that properly dealing with any rain accumulation, if we do keep that, is a larger scope discussion.)
Contributor

zerog2k commented Jan 7, 2017

Ok a few updates:

  • register dummy template in place of old removed acurite5n1 driver to prevent device reordering
  • removed references to old driver (specifically, old, probably incorrect wind direction mapping). (I have verified new driver, acurite_txr, is correct with my original vn1tx 5n1 station.)
  • move message types to defines
  • output raw rain counter value in addition to accumulated value (I'll open a separate issue for the one bug I did find with this, which is wrong accumulation with more than one station reporting - however this might turn out to be quite complex - it would probably need state tracking per-sensor id to prevent collision. I feel that properly dealing with any rain accumulation, if we do keep that, is a larger scope discussion.)
@matthewwall

This comment has been minimized.

Show comment
Hide comment
@matthewwall

matthewwall Jan 8, 2017

i don't know about other consumers, but weewx can tell the difference between different 5in1 clusters, thanks to the sensor identifiers that rtl_433 includes in each packet.

the weewx-sdr extension ignores the 'Total rain fall since last reset' messages, and uses the value from the 'rain gauge' in type 31 messages as an accumulated rain value (from which weewx derives rain_delta, rain_since_midnight, etc).

https://github.com/matthewwall/weewx-sdr/blob/master/bin/user/sdr.py

matthewwall commented Jan 8, 2017

i don't know about other consumers, but weewx can tell the difference between different 5in1 clusters, thanks to the sensor identifiers that rtl_433 includes in each packet.

the weewx-sdr extension ignores the 'Total rain fall since last reset' messages, and uses the value from the 'rain gauge' in type 31 messages as an accumulated rain value (from which weewx derives rain_delta, rain_since_midnight, etc).

https://github.com/matthewwall/weewx-sdr/blob/master/bin/user/sdr.py

@rct

A few more things. My apologies I had this on my list to do a while back when I'm near my 5n1.

Show outdated Hide outdated src/devices/acurite.c Outdated
// 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

This comment has been minimized.

@rct

rct Jan 8, 2017

Contributor

(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.

@rct

rct Jan 8, 2017

Contributor

(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.

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];

This comment has been minimized.

@rct

rct Jan 8, 2017

Contributor

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.

@rct

rct Jan 8, 2017

Contributor

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.

Show outdated Hide outdated src/devices/acurite.c Outdated
@zerog2k

This comment has been minimized.

Show comment
Hide comment
@zerog2k

zerog2k Jan 8, 2017

Contributor

per @rct reminder, I have moved all units back to customary (US/imperial?), as at least two native device output formats (temperature and rainfall increments) are not in SI units.
The only thing I noticed looking through the unit conversion code is that it handles temp_C/temp_F conversion.
https://github.com/merbanan/rtl_433/blob/master/src/rtl_433.c#L224-L261
I did not see where it handles kph/mph conversion... maybe this is yet to be implemented for other types of units (speed, distance, etc).

Contributor

zerog2k commented Jan 8, 2017

per @rct reminder, I have moved all units back to customary (US/imperial?), as at least two native device output formats (temperature and rainfall increments) are not in SI units.
The only thing I noticed looking through the unit conversion code is that it handles temp_C/temp_F conversion.
https://github.com/merbanan/rtl_433/blob/master/src/rtl_433.c#L224-L261
I did not see where it handles kph/mph conversion... maybe this is yet to be implemented for other types of units (speed, distance, etc).

@zerog2k

This comment has been minimized.

Show comment
Hide comment
@zerog2k

zerog2k Jan 10, 2017

Contributor

Here is the output with json formatting on, and two acurite 5n1 sensors reporting:
https://gist.githubusercontent.com/zerog2k/6340a71252015d1032314b7e99d0b6e9/raw/e4bde902d8813a66d6b9935bc579be739d3b0742/gistfile1.txt
(note that they are disagreeing in values because the sensor_id 3585 is inside my house, the other sensor is actually mounted up outside and measuring real weather.)

Contributor

zerog2k commented Jan 10, 2017

Here is the output with json formatting on, and two acurite 5n1 sensors reporting:
https://gist.githubusercontent.com/zerog2k/6340a71252015d1032314b7e99d0b6e9/raw/e4bde902d8813a66d6b9935bc579be739d3b0742/gistfile1.txt
(note that they are disagreeing in values because the sensor_id 3585 is inside my house, the other sensor is actually mounted up outside and measuring real weather.)

@zerog2k

This comment has been minimized.

Show comment
Hide comment
@zerog2k

zerog2k Jan 13, 2017

Contributor

@merbanan @rct @matthewwall
I think I have addressed all the main issues brought up in this review. Is there anything else we missed?

Regarding the rain accumulation code, I suggest we take what to do up as a separate issue/PR.

Contributor

zerog2k commented Jan 13, 2017

@merbanan @rct @matthewwall
I think I have addressed all the main issues brought up in this review. Is there anything else we missed?

Regarding the rain accumulation code, I suggest we take what to do up as a separate issue/PR.

@merbanan merbanan merged commit 0bb4ac2 into merbanan:master Jan 13, 2017

@merbanan

This comment has been minimized.

Show comment
Hide comment
@merbanan

merbanan Jan 13, 2017

Owner

Ok, this looks better then before.

Owner

merbanan commented Jan 13, 2017

Ok, this looks better then before.

@eras

This comment has been minimized.

Show comment
Hide comment
@eras

eras Mar 17, 2017

Contributor

Just accidentally saw this. Not introduced by this change but could this be a bug? The file has two versions of expression battery_low ? "OK" : "LOW". If the condition is correct then the battery_low name is a bug ;-).

Contributor

eras commented on src/devices/acurite.c in 33aabf6 Mar 17, 2017

Just accidentally saw this. Not introduced by this change but could this be a bug? The file has two versions of expression battery_low ? "OK" : "LOW". If the condition is correct then the battery_low name is a bug ;-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment