-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 Blueline PowerCost Monitor #1580
Conversation
This comment has been minimized.
This comment has been minimized.
Quick notes: yes, decoders must not be stateful. I need to look into this in detail later, but we really don't want any static state in decoders. |
src/devices/blueline.c
Outdated
// If BLUELINE_ACTIVE_ID_AUTODETECT is set to 1, this code will try to autodetect IDs from | ||
// incoming messages which otherwise would fail CRC. While there are a lot of false positives | ||
// on each individual message, only one ID will continuously match up, and it becomes | ||
// obvious which one is the right one within a few minutes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many matches do you get? Is there a way to validate the payload parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any individual message will get 2^6 matches. The payload can unfortunately be any value and there's no way to narrow it down by looking for reserved or unused fields.
@jbrzozoski regarding recovering the correct id, if it still is almost real time on most platforms it is ok. But you must be as sure as possible before we try to recover the id. The sparsnas decoder uses something similar. |
Regarding the temperature. If you post a set of readings Im sure we can figure it out. |
I now remember what I thought initially. Each transmission contains 3 payloads that might be different. I think that if all payloads are used then it should be possible to recover a unique id. But that should not be the case based on comments I've read. So I think we need to pass the id by command line to the decoder. Or accept that this decoder needs a state. The raw logic is that we got a 14 bit id and only 8 bits of crc to match it against. (And the crc is supposed to validate the payload, not to be used to recover the id, a broken payload will recover a bad id). |
Within any given burst, all 3 messages are often the same message repeated, so it's not practical to expect to be able to recover the ID from one burst. Your summary that we need to either pass the ID from the command line or maintain a state was my conclusion as well, but I couldn't find any examples of other devices with custom command line options, so I tried to avoid that. |
https://github.com/merbanan/rtl_433/blob/master/src/devices/fineoffset.c#L20 I suggest you add one option that sets the key and another option to brute force the key (maybe you can not reset the transmitter). That option will make the decoder keeping a state. I think that is an ok compromise. By default I prefer things to be stateless. @zuckschwerdt I think this is a good enough compromise. A user could enable to option and things would just work. If anyone requests support the answer will be that they need to figure out the key for things to work. And for regression tests we can either add the key to the command line or just handle the message with a proper crc. |
I like the two options plan. A user can interactively use the option to brute force the key, and within a couple minutes rtl_433 should start reporting data including the ID key that it found. Then the user can stop that interactive session and just pass in the discovered key on the command line for all future use. Not having the stateful key search running all the time avoids some weird issues that could crop up if there happens to be two Blueline transmitters within range, or a single rtl_433 session running so long that random noise eventually looks like a valid transmitter. The brute force search mechanism has no concept of time. If enough random messages all match a key over several days without restarting rtl_433, it would consider that a match and switch to a bogus ID, which would then switch back quickly if a real transmitter is in range. |
Slight tweaking to behavior... If you don't pass any parameters to this decoder, all it will ever do is spit out the TXID message if it hears someone restart a monitor. And I suppose if you just happened to have a monitor ID of 0 it would work, too, but that's unlikely. You can pass in the special parameter "auto" which will put it in auto-detect mode until the first time it sees a TXID message or the brute-force passes the threshold, at which point it will switch to using that ID and turn off the autodetect. (To avoid any of the weird issues I mentioned earlier in this thread.) Finally, any other parameter passed in is assumed to be an ID, and just passed through strtoul. I like this since it actually can support multiple transmitters in range if you just pass multiple registrations on the command line like All state is setup at creation time except when "auto" mode is enabled, and once that picks an ID it also stops changing state. This kind of what you were thinking, @merbanan ? |
src/devices/blueline.c
Outdated
working_buffer[1] += 1; | ||
} | ||
|
||
fprintf(stderr, "Attempting Blueline autodetect: best_hits=%u\n", best_hits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be behind verbosity check.
src/devices/blueline.c
Outdated
|
||
// TODO - Add comments for the new parameters and expected usage | ||
|
||
#include <stdbool.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No stdbool.h please.
src/devices/blueline.c
Outdated
#define MAX_POSSIBLE_BLUELINE_IDS (65536/BLUELINE_ID_STEP_SIZE) | ||
#define BLUELINE_ID_GUESS_THRESHOLD 10 | ||
|
||
struct blueline_context { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name it blueline_stateful_context, the it will be very clear what this is.
src/devices/blueline.c
Outdated
decoder_output_data(decoder, data); | ||
payloads_decoded++; | ||
if (context->searching_for_new_id) { | ||
fprintf(stderr,"Switching to received Blueline ID 0x%04X\n", received_sensor_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verbose guard missing.
src/devices/blueline.c
Outdated
if ((context->searching_for_new_id) && (message_type != BLUELINE_TXID_MSG)) { | ||
uint16_t id_guess = guess_blueline_id(context, current_row); | ||
if (id_guess != 0) { | ||
fprintf(stderr,"Switching to auto-detected Blueline ID 0x%04X\n", id_guess); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verbose guard missing.
src/devices/blueline.c
Outdated
if (strcmp(arg, "auto") == 0) { | ||
// Setup for auto identification | ||
context->searching_for_new_id = true; | ||
fprintf(stderr, "Blueline decoder will try to autodetect ID.\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verbose guard missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing any way to determine if we're in verbose mode inside the creator functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd like, I can add a commit into this pull request to add more parameters to the create_fn, either the whole of the r_cfg_t or just the verbosity value from it. I'll adjust fineoffset.c to match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the output for now. Lets focus on getting this patch in first. Then we can add it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't access r_cfg from decoders -- we need some modularity.
src/devices/blueline.c
Outdated
} else { | ||
// Assume user is trying to pass in hex ID | ||
context->current_sensor_id = strtoul(arg, NULL, 16); | ||
fprintf(stderr, "Blueline decoder using ID %04X\n", context->current_sensor_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verbose guard missing.
So the command -R 176:auto would enable the brute force mode? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I received confirmation from BLI that the Black & Decker EM100B that looks identical to this unit i functionally identical and should be compatible with this library. When this work is completed, it should be noted that it is also usable. |
Just some minor tweaks in the right direction:
And to answer your question, using |
(Just pushing to fix a style check) |
src/devices/blueline.c
Outdated
|
||
#ifndef UNUSED | ||
#define UNUSED(x) (void)(x) | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. That's what it is. 🤣
(Yes, I'll remove it.)
src/devices/blueline.c
Outdated
.short_width = 500, | ||
.long_width = 1000, | ||
.gap_limit = 2000, | ||
.reset_limit = 200000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the practical limit for this is much smaller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 8000 would work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's almost a don't-care then, as long as it's bigger than gap_limit. The pauses between rows in one burst is actually closer to 100000, so they always show up in the decoder one row at a time. I'll bring this down to 8000.
Get a good set of sample signals and submit it to the rtl_433_tests repo. Then we try to figure out the temperature encoding and then I think we are set. The code looks fine to me. @zuckschwerdt ok to merge after those fixes? |
Signed-off-by: Justin Brzozoski <justin.brzozoski@gmail.com>
I'll submit some samples later today after I can head outside and hit the button a few times to make sure I have samples of the ID message. |
Samples are up: |
I don't know why, but none of the verbose logging seems to be working. I guess this has something to do with rtl_433 internals? Otherwise this seems to work well with the submitted samples. Have not thoroughly reviewed it, but the code looks good in general. |
I noticed the verbose output wasn't working when I was testing the recorded
samples, but assumed it was somehow a side affect of the replay mechanism.
I just double checked on my receiver and it doesn't work there either. I'll
take a closer look in the next 5-7 hours.
|
Verbosity and registrations are command line order dependent:
@Mindavi do you know which order you had your command line setup? |
Is there a way to pass a parameter to one decoder without disabling all the others? I'm seeing that just using |
Not really recommended but |
Yes, I had the verbose parameter at the end of the command. However, I can't get the verbose output working with the parameters anywhere on the line. Did you get it working? Edit: whoops, I have to add auto to the line, of course. That works with the |
Not really the ideal way to test it, buy you can prove the "auto" mode works with the samples I provided. You just have to pass each file multiple times to a single instance, so something like this will exercise the brute force method: I wasn't really checking for it when I made the samples, but the CRCs in those payloads hit enough orthogonal IDs that only the right one matches all of them and passes the threshold even if they're repeated. (i.e. no false positives) If you put it in "auto" mode and use the ID sample, it will immediately lock onto the ID payload and will decode everything after that, so a command line like this will decode all of the samples: |
Yeah, I figured it worked like that. Seems to work fine. Are there any open comments here? |
Config and manual additions are missing but besides that everything is ok by me. |
Did you need anything else from me on this? And my main monitoring station now has these config lines:
Which looks weird, but works fine setting an option on this decoder while leaving all default decoders enabled. 👍 |
This is still a work in progress, but it is nominally working and I would like other people's feedback.
Open questions and things to do: