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

Added protocol for Dickert MAHS garage door remote control #3826

Merged
merged 12 commits into from
Aug 9, 2024

Conversation

OevreFlataeker
Copy link
Contributor

@OevreFlataeker OevreFlataeker commented Aug 7, 2024

What's new

  • This commit adds the protocol for the 433.92 MHz garage door remote control from Dickert, a German vendor of garage door systems. The protocol only support the linear code types which have 10 user controllable tri-state DIP switches (+,0,-) . In addition a factory code can be customized (by the vendor) which adds further 8 symbols to the complete code.
  • Technical details about the radio signal can be found here: Dickert MAHS433-01 garage/gate remote decoding merbanan/rtl_433#2983
  • The basis of this code is the CAME protocol implementation which I extended for the needed changes for this protocol.

Verification

  • UnitTests + Manual testing based on files provided in unit tests assets folder

Checklist (For Reviewer)

  • PR has description of feature/bug or link to Confluence/Jira task
  • Description contains actions to verify feature/bugfix
  • I've built this code, uploaded it to the device and verified feature/bugfix

Here is an example file for this signal: MAHS_w_FacCode.sub.zip

Added Dickert MAHS protocol
Added Dickert MAHS protocol reference
hedger
hedger previously requested changes Aug 7, 2024
Copy link
Member

@hedger hedger left a comment

Choose a reason for hiding this comment

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

Please remove file i/o, clean up commented out code and run code formatter (fbt format).

@hedger hedger added Sub-GHz Sub-GHz-related New Feature Contains an IMPLEMENTATION of a new feature labels Aug 7, 2024
@skotopes
Copy link
Member

skotopes commented Aug 8, 2024

Looks like something wrong with encoder/decoder: replayed emulated signal record is not recognized.
You can reproduce it using second flipper or write unit tests for the protocol(see https://github.com/flipperdevices/flipperzero-firmware/blob/dev/applications/debug/unit_tests/tests/subghz/subghz_test.c).

@skotopes skotopes marked this pull request as draft August 8, 2024 06:27
@skotopes
Copy link
Member

skotopes commented Aug 8, 2024

Un-draft when ready

@OevreFlataeker
Copy link
Contributor Author

Will check. Did you use my provided sample file for testing the replaying?

@skotopes
Copy link
Member

skotopes commented Aug 8, 2024

Will check. Did you use my provided sample file for testing the replaying?

yes

@OevreFlataeker
Copy link
Contributor Author

Ok, and what do I need to do exactly to continue the process once I pushed a fix? "Request review" again?

@skotopes
Copy link
Member

skotopes commented Aug 8, 2024

Ok, and what do I need to do exactly to continue the process once I pushed a fix? "Request review" again?

image

@OevreFlataeker
Copy link
Contributor Author

OevreFlataeker commented Aug 8, 2024

Verifying my transmissions now with URH and comparing to a real handheld sender.

It seems the very first transmission always starts with a 111000 sequence, all repeated transmissions do not have that and are perfectly 1:1 what I intend to send. Any idea where this preamble is coming from? Is this a setting in the CC1101 I need to change maybe?

image

@OevreFlataeker
Copy link
Contributor Author

I've added a unit test for my encoder and "as expected" it fails. I presume it's due to the 111000 preamble, that I don´t know where this is coming from (see above). Could I kindly get some support from @skotopes to get rid of that preamble?

@OevreFlataeker
Copy link
Contributor Author

The screenshot above was generated by recording with an RTL SDR and URH while doing Subghz->Saved->Select my sub file->Emulate->Keep button pressed for 1-2 seconds.

@OevreFlataeker
Copy link
Contributor Author

Still not sure what's up with those preamble bytes - is it the data whitening feature of the cc1101 maybe?? - but I got the unit_tests to pass both the encode and decode tests with the provided test files.

@OevreFlataeker OevreFlataeker marked this pull request as ready for review August 8, 2024 22:53
@skotopes
Copy link
Member

skotopes commented Aug 8, 2024

There are no cc1101 settings that you can modify from encoder/decoder. All data is formed on your side.

@OevreFlataeker
Copy link
Contributor Author

All requested changes by hedger should now be implemented I think. Anything else I need to do before it can be merged?

@skotopes
Copy link
Member

skotopes commented Aug 9, 2024

I'll do test, review and then merge. However it will be included in next release.

@skotopes skotopes merged commit 4f75d92 into flipperdevices:dev Aug 9, 2024
11 checks passed
@OevreFlataeker
Copy link
Contributor Author

@skotopes I did some more tests in order to understand where the 111000 prefix when the transmission starts is coming from. I used the holtek.sub from the unit_tests application and emulated this using the SubGhz app interactively. I chose this one, because it appears to be similar to my sub file/protocol when sending.

For example in subghz_protocol_encoder_holtek_get_upload there is also this silence+start bit transmission before the actual data is send:

static bool subghz_protocol_encoder_holtek_get_upload(SubGhzProtocolEncoderHoltek* instance) {
    furi_assert(instance);

    size_t index = 0;
    size_t size_upload = (instance->generic.data_count_bit * 2) + 2;
    if(size_upload > instance->encoder.size_upload) {
        FURI_LOG_E(TAG, "Size upload exceeds allocated encoder buffer.");
        return false;
    } else {
        instance->encoder.size_upload = size_upload;
    }

    //Send header
    instance->encoder.upload[index++] =
        level_duration_make(false, (uint32_t)subghz_protocol_holtek_const.te_short * 36);
    //Send start bit
    instance->encoder.upload[index++] =
        level_duration_make(true, (uint32_t)subghz_protocol_holtek_const.te_short);
    //Send key data
    for(uint8_t i = instance->generic.data_count_bit; i > 0; i--) {
        if(bit_read(instance->generic.data, i - 1)) {
...

... and much to my surprise I see the same preamble when recording the transmission with URH!

image

So I still want to understand where this 111000 is coming from. Is this something that is always send at the beginning of a transmission in the SubGhz app? Can you (@skotopes) explain why this is happening? (And where in the code is this done? I searched high and low yesterday where exactly the I/O with the CC1101 is actually happening) Thank you very much for your time.

@skotopes
Copy link
Member

skotopes commented Aug 9, 2024

@Skorpionm would you kindly?

@Skorpionm
Copy link
Member

give me Read_Raw recording, original remote control 5+ presses for 3+ seconds. for all buttons and with pre-known switch positions. if you are sure that you are decoding the position of the switches correctly, then recording with different positions of the Dip switches is not necessary

@Skorpionm
Copy link
Member

image
Anything is possible without providing Read_Raw.
You start signal transmission from a low level. When implementing signal transmission from Wb55 -> CC1101, the transmission must start from a high level, otherwise all durations will be “inverted”. Middleware inside the CC1101 driver injects high-level duration and your transmission is already coming from it, it does not affect the result

@OevreFlataeker
Copy link
Contributor Author

OevreFlataeker commented Aug 9, 2024

...CC1101, the transmission must start from a high level, otherwise all durations will be “inverted”. Middleware inside the CC1101 driver injects high-level duration and your transmission is already coming from it, it does not affect the result

But during the decoding would the 111000 not hinder the proper decoding as the decoder expects a quite long period of silence before it will detect the "start bit"? Same as the "holtek" decoder who does basically the same thing? Wouldn´t that mean, the very first transmission is always not detected as this has 111000 whereas all the other's don´t have that? Or am I misunderstanding your explanation maybe?

@OevreFlataeker
Copy link
Contributor Author

OevreFlataeker commented Aug 9, 2024

give me Read_Raw recording, original remote control 5+ presses for 3+ seconds. for all buttons and with pre-known switch positions. if you are sure that you are decoding the position of the switches correctly, then recording with different positions of the Dip switches is not necessary

Read raw recording: 5 presses of 3 sec. RSSI filter set to -50
Screenshot-20240809-141810

5x3.zip

Properly decoded (DIP positions match the decoding)

>: subghz decode_raw /ext/subghz/5x3.sub
Load_keystore keeloq_mfcodes OK
Load_keystore keeloq_mfcodes_user Absent
Listening at /ext/subghz/5x3.sub.

Press CTRL+C to stop

Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000
Dickert_MAHS
User-Dips:      +00-+---0+
Fac-Code:       0000-000

Packets received 20

@OevreFlataeker
Copy link
Contributor Author

...CC1101, the transmission must start from a high level, otherwise all durations will be “inverted”. Middleware inside the CC1101 driver injects high-level duration and your transmission is already coming from it, it does not affect the result

Could you kindly where in the code ind the middleware this injecting is done?

@OevreFlataeker
Copy link
Contributor Author

OevreFlataeker commented Aug 9, 2024

https://github.com/flipperdevices/flipperzero-firmware/pull/3826/files#diff-b26b1b764469c606a87589f3443f3d62fd99933f7c6f81625e818c67fb3d3657

this file looks very badly cut.

Happy to improve it if you could kindly tell me how. Are the raw sub files for the unit tests supposed to only contain one perfect transmission without any noise around it?

@Skorpionm
Copy link
Member

static inline uint32_t furi_hal_subghz_async_tx_middleware_get_duration(

here. if the decoder is written correctly, it starts parsing only when it sees a long low, no matter when it happens

@Skorpionm
Copy link
Member

for unit tests you need a simple recording of the broadcast, not perfectly processed, but just as it is on the air

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Feature Contains an IMPLEMENTATION of a new feature Sub-GHz Sub-GHz-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants