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

[EXPERIMENTAL] Argo WREM2 - changed proto msg size: 96->90 bits #2136

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mbronk
Copy link
Contributor

@mbronk mbronk commented Aug 30, 2024

Includes matchBytes() change to be able to work with non-byte-aligned bit lengths.

Status: EXPERIMENTAL

Based off #2133 captures and the fact the last 6 bits were unused/past checksum, looks like true proto length may be 90-bit (vs. previous value of 96), hence the adjustment.
I do NOT own any WREM2 device so unable to tell if this violates any backwards compatibility and whether the captures in #2133 are the WREM2 or if there's a 96-bit variant out there as well.

@mbronk mbronk force-pushed the private/mbronk/EXPERIMENTAL_issue2133_fix branch from ab1eac1 to 31808d8 Compare August 30, 2024 13:59
@mbronk
Copy link
Contributor Author

mbronk commented Aug 30, 2024

@crankyoldgit , @NiKiZe - this is not a full/mergeable PR (don't have a device to test with), rather an experimental update, aiming to provide a step towards fixing #2133. I am deliberately KEEPING it in Draft ❕, as I think it will require some follow up, and unsure if I'll have any bandwidth in the near future. If it proves to have any merit, hope you can join-in and co-author.

  • [EDIT] The issue author confirmed it working on his setup and I've captured captures from Argo Ulisse 13 IR messages not decoding correctly #1859 as UTs (seem to pass), so am going to flip this PR to READY b/c it might 🎲 just as well work. All caveats from before apply though - there may well be areas where it breaks. Do with these changes as you please.

Highlights

  1. Unsure if just changing the protocol size 96->90 bit is safe. Based on:

    uint32_t :6;

    it may be, but it is equally possible we're dealing with a new/different protocol flavor.

  2. I'm thinking this code

    0, 0, // Footer (None, allegedly. This seems very wrong.)

    may actually be safe to fix (use an actual footer). Didn't touch it last time, assuming it does sth useful, but if making WREM2 changes... it looks like a candidate to change as well.

  3. The change introducing non-byte-aligned parsing in IRrecv.cpp probably need a 2nd look if the direction is right (likely: more UT) as it removes the strictness the function had before (non-byte-aligned bit counts were not supported above 64 bits).

Includes `matchBytes()` change to be able to work with non-byte-aligned bit lengths.

Signed-off-by: Mateusz Bronk <[email protected]>
@mbronk mbronk force-pushed the private/mbronk/EXPERIMENTAL_issue2133_fix branch from 31808d8 to 6461a31 Compare August 30, 2024 14:07
mbronk added 2 commits August 31, 2024 15:14
From captures in issue crankyoldgit#2133 (confirmed valid by the OP)

Signed-off-by: Mateusz Bronk <[email protected]>
Reason: OP confirmed the temp value readings are correct.

Signed-off-by: Mateusz Bronk <[email protected]>
@crankyoldgit crankyoldgit self-requested a review August 31, 2024 14:08
@crankyoldgit crankyoldgit self-assigned this Aug 31, 2024
@crankyoldgit
Copy link
Owner

I'll try to review this on Tuesday (GMT-10). I'm busy till then.

From captures in issue crankyoldgit#1859

Signed-off-by: Mateusz Bronk <[email protected]>
@mbronk mbronk marked this pull request as ready for review August 31, 2024 14:53
@mbronk
Copy link
Contributor Author

mbronk commented Oct 6, 2024

@crankyoldgit - any updates?

Copy link
Collaborator

@NiKiZe NiKiZe left a comment

Choose a reason for hiding this comment

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

I think this seems ok, I'm a bit curios on how/if this affects performance.

I'm also wondering if this will affect other protocols, or if some protocol implementations could be simplified after this change.

But those are just notes for future things to look into if/when time permits.

Copy link
Owner

@crankyoldgit crankyoldgit left a comment

Choose a reason for hiding this comment

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

Sorry about that.

I'm a little uneasy at changing the 8-bit nature of the _matchGeneric

For protcols that have "odd" protocol lengths. i.e. Well over 64 bits and not a multiple of 8 (e.g. Kelvinator & Gree). I've found that some of the message has a fixed few bits. e.g. 3 odd bits in Kelvinator.

Is that the case here?

@mbronk
Copy link
Contributor Author

mbronk commented Oct 6, 2024

Is that the case here?

As far as this concrete protocol goes, the trailer is variable. Messages end with 8-bit checksum, which crosses bytes 10 (6 bits) and 11 (2 bits).
Ref:

uint32_t Sum :8; // straddle byte 10 and 11

@NiKiZe
Copy link
Collaborator

NiKiZe commented Oct 6, 2024

Is that the case here?

As far as this concrete protocol goes, the trailer is variable. Messages end with 8-bit checksum, which crosses bytes 10 (6 bits) and 11 (2 bits). Ref:

uint32_t Sum :8; // straddle byte 10 and 11

It would be nice if there is 2 bits that is constant, we could then assume those are part of message format.

If there is a CRC of 8bits, then it is odd if those are not byte aligned.

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

Successfully merging this pull request may close these issues.

3 participants