Skip to content
This repository has been archived by the owner on Nov 19, 2024. It is now read-only.

SubGhz: Fix Honeywell Security and TX141THBv2 save/load #513

Closed
X3r0C00L opened this issue Jan 9, 2024 · 10 comments · Fixed by #540
Closed

SubGhz: Fix Honeywell Security and TX141THBv2 save/load #513

X3r0C00L opened this issue Jan 9, 2024 · 10 comments · Fixed by #540
Assignees
Labels
bug Something isn't working release-pending This has been implemented and is waiting to be released publicly

Comments

@X3r0C00L
Copy link

X3r0C00L commented Jan 9, 2024

Describe the bug.

Ive use the read function under subghz on 345mhz and read some honeywell security protocol signals. But when I save them and go to reopen them it either opens it up but all the data are zeros, or it wont open and says protocol missing.
This is on latest version of xtreme.
Ive went back and recorded the signal raw and then transmitted it to my other flipper and read it and saved it and same issue.

Reproduction

Read signal in SubGHZ, itll display honeywell security protocol. view signal and it looks good. shows serial number channel etc.
Save signal and then try to reopen it and it fails.

Target

No response

Logs

No response

Anything else?

No response

@X3r0C00L X3r0C00L added the bug Something isn't working label Jan 9, 2024
@X3r0C00L
Copy link
Author

Would like to add that there seems to be another issue involved with this protocol. If you read the signal (not raw) and it comes up as Honeywell Security, then open it to inspect it will give you the serial number, channel etc.
If you transmit that signal to another flipper in read mode (Not Raw) The data that the other flipper receives does not match what was sent most of the time. Sometimes it does, but mostly not.

@Willy-JL
Copy link
Contributor

cc @htotoo

i tried to make sense of the code but don't see anything inherently wrong. only thing is maybe the bit count, saw that one of the two deserialize functions for honeywell sec protocol enforces bit count, while some of your comments mention something about 62 bits + 2 hardcoded at the start? maybe its saved with all 64 bits but checked for == 62 bits since min_count_bit_for_found = 62 and that is passed to subghz_block_generic_deserialize_check_count_bit() ?
to be fair i dont see min_count_bit_for_found used anywhere else in honeywell sec. its set in the header file, and passed to deserialize to check count.
or maybe its saved as 62 bits, loaded as 62 bits, but the parser expects 64 bits since that is how it works while being received originally and so it parses data wrong due to being 2 bits off?

no clue, just giving ideas, altho the last one seems the most logical explanation to me

if you can provide a raw file i can try to investigate further @X3r0C00L , i dont have access to a system using this protocol. feel free to message me on our discord if you prefer

while im pinging you @htotoo , i also wanted to ask a few things about the protocol integrations you made in #399 :

  • i noticed an issue with TX141THBv2, it saves but then i cant open the file back up at all. i have since accidentally deleted the file, but i dont see anything wrong at a first delve in the code.
  • all the weather station protocols added have added flags load and save (compared to the originals in the weather station fap), while LaCrosse_TX doesnt have them. any specific reason why?
  • Nexus-TH has different timings compared to the ones in the original fap (490 vs 500, 1980 vs 2000), why?

@Willy-JL
Copy link
Contributor

Willy-JL commented Jan 13, 2024

actually just found a raw file from the bounty channel first asking for honeywell sec to be implemented, can replicate the issue. ill try to figure this out but no promises, still easier if we hear back from htotoo probably.

also found example files for TX141THBv2 here, noticed that 40bit loads, 41bit doesnt, my guess is its the same check for bit count thats the issue, ill see now

@Willy-JL Willy-JL changed the title SubGHZ Honeywell Security Protocol corrupting when saving. SubGhz: Fix Honeywell Security and TX141THBv2 save/load Jan 13, 2024
@Willy-JL Willy-JL moved this to 🏗 In progress in Xtreme-Firmware Jan 13, 2024
@Willy-JL Willy-JL self-assigned this Jan 13, 2024
@X3r0C00L
Copy link
Author

DoorSensor.txt
Here is the Raw Sub.
Youll have to change the extension back to .sub I had to change it to txt in order to upload on here.

@Willy-JL
Copy link
Contributor

Willy-JL commented Jan 13, 2024

i think i have found the issue. it is not a bit count issue, its a logic issue.

when decoding in real time, the decoder is fed data received and tries to match the protocol and understand it. when it succeeds, it copies the result to generic data instance. then when saving, the generic helper uses generic instance to save to file. then when loading from file, it loads into generic instance. issue is that the protocol only copies from decode into generic on successful decode, and load always puts it into generic, while the rest of the protocol always reads from decode data, not from generic data. it works right after receiving only because th decoded data is not reset. however i suspect that in current state, if you receive 2 different honeywell sec signals, no matter which one you open from the history, it will show the data of latest one, since it is the last one to be decoded, and protocol looks at decoded data not loaded data.

looking at how other protocols handle this, it seems like decode data buffer should only be used when decoding, and copied into generic data right after successful decode. after that, all logic should read from generic data. this way, even loading from data allows protocol to function correctly.

also an update on TX141THBv2, it was indeed a 40/41 bit conflict issue, i have fixed this, will push soon

@Willy-JL
Copy link
Contributor

actually i was partially wrong. the protocol does only use decode data while decoding, then later always reads from generic data. the issue is that the decode fills in the specific protocol info like serial number, btn, heartbeat and so on that are used to show info on screen, while load from file doesnt.

quick look at the code makes me think that this is onloy a visual glitch, atleast the serial number stored after decoding is not used anywhere except in showing info on screen, so maybe sending it still works as normal? can you try that @X3r0C00L ?

@Willy-JL
Copy link
Contributor

either way its fixed now

@Willy-JL Willy-JL added the release-pending This has been implemented and is waiting to be released publicly label Jan 13, 2024
@Willy-JL Willy-JL moved this from 🏗 In progress to 👀 Done (In next release) in Xtreme-Firmware Jan 13, 2024
@htotoo
Copy link
Contributor

htotoo commented Jan 13, 2024

  • min_count_bit_for_found is intentional, because sometimes it sends data badly, and the remaining bytes are good, so even if the proper protocol data is 64, i ignored the missing ones. That's why i look for multiple preambles in line 67.
  • LaCrosse_TX missing load+save seems to be a mistake as i can recall, that should have it too.
  • Nexus-TH is the weather proto i have device for, it worked better with 500, based on my captures and checked it with URH.

When it is made I tested the load and save (for Honeywell) it worked, and the structure is from another weather proto, so there maybe another issue with loading (if i didn't messed up something, and didn't noticed).

Anyway, thanks for looking into it and fixed!

@Willy-JL
Copy link
Contributor

perfect, thanks

@X3r0C00L
Copy link
Author

Thanks a bunch. Super awesome work.

@Willy-JL Willy-JL linked a pull request Feb 2, 2024 that will close this issue
@Willy-JL Willy-JL closed this as completed Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working release-pending This has been implemented and is waiting to be released publicly
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants