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

Add support for ShellyBLU Wall Switch4 and RC Button4 devices #564

Merged
merged 20 commits into from
Oct 15, 2024

Conversation

larswnd
Copy link
Contributor

@larswnd larswnd commented Sep 12, 2024

  • Added documentation for ShellyBLU Wall Switch4 (SBBT-004CEU) and RC Button4 (SBBT-004CUS).
  • Included JSON definitions and properties for SBBT-004CEU and SBBT-004CUS devices.
  • Updated devices.h and decoder.h to include new device models and their encrypted versions.
  • Added test cases for ShellyBLU RC Button4 in test_ble.cpp.

Description:

I own an SBBT-004CUS (The EU one seems identical in the [documentation] (https://shelly-api-docs.shelly.cloud/docs-ble/Devices/wall_us)
Some things to consider:

  • I did not try to build, run or test anything, I am sorry, but currently I do not have enough time to dig into this very deep
  • I do not own a EU switch, so there are no tests
  • I do not want to use the app, so I don't have encrypted data, but documentation is identical to SBBT-002C, so it should work
  • All ShellyBLU Devices have the same Bluetooth name as their Device Name, I always get an different one, so I used that (for US only)

I hope you can help me finishing this pull request.

Checklist:

  • The pull request is done against the latest development branch
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • I accept the DCO.

Lars added 6 commits September 12, 2024 15:23
- Added documentation for ShellyBLU Wall Switch4 (SBBT-004CEU) and RC Button4 (SBBT-004CUS).
- Included JSON definitions and properties for SBBT-004CEU and SBBT-004CUS devices.
- Updated `devices.h` and `decoder.h` to include new device models and their encrypted versions.
- Added test cases for ShellyBLU RC Button4 in `test_ble.cpp`.
@DigiH
Copy link
Member

DigiH commented Sep 12, 2024

Hi @larswnd - Thanks for this.

• I do not own a EU switch, so there are no tests

Would you mind adding some unit tests for the EU switch as well, from the decoder logic you created, similar to the US one. This ensures regression testing during future Decoder additions.

• All ShellyBLU Devices have the same Bluetooth name as their Device Name, I always get an different one, so I used that (for US only)

In your model conditions you use the full name, including part of your specific device's MAC address, for comparison. This means the decoder will only work with this single "SBBT-US76d2" device with MAC address 38:39:8F:71:76:D2 . You should change this to just the static leading characters, identical to all "SBBT-US…", like you did for the "SBBT-004CEU", although I would imagine that this might also need to be changed to "SBBT-EU", if the name broadcast is similar along the lines of the SBBT-004CUS broadcasting "SBBT-US…".

I will have the time to take a more detailed look on the weekend, to see why there are still some build failures.

Lars added 7 commits September 12, 2024 20:15
- Modify `SBBT_004CEU_json.h` to update the model name from "SBBT-004CEU" to "SBBT-EU".
- Modify `SBBT_004CUS_json.h` to update the model name from "SBBT-US76d2" to "SBBT-US".
- Modify `SBBT_004CEU_ENCR_json.h` to update the model name from "SBBT-004CEU" to "SBBT-EU".
- Modify `SBBT_004CUS_ENCR_json.h` to update the model name from "SBBT-US76d2" to "SBBT-US".
- Ensure consistency in JSON property definitions across all device headers.
@larswnd
Copy link
Contributor Author

larswnd commented Sep 12, 2024

I added some Unit Tests for the EU model.

Good point with the mac in the name. I was blind... felt wrong.
I removed the MAC Part inside the name and used SBBT-US, SBBT-EU

I saw some
ERROR property condition data source invalid
and some
Invalid data SBBT-US; skipping
in the tests/checks. Those are also there for other device, and tests do not fail anymore. So it could be fine?

@DigiH
Copy link
Member

DigiH commented Sep 14, 2024

Hi @larswnd

I had the time to look at this more closely today.

The unencrypted decoders do look fine to me from here, not having any Shelly devices myself, and with the added test cases and assuming that you have also tested and verified this with your actual SBBT-004CUS they look ready to go.

While I do understand your reluctance to install and use any proprietary app, the encrypted decoders look wrong to me. You state

I don't have encrypted data, but documentation is identical to SBBT-002C, so it should work

This is correct for the actual button press encoding, but the SBBT-002C only has one single button, while the ShellyBLU Wall Switch4 (SBBT-004CEU) and RC Button4 (SBBT-004CUS) have four buttons. Because of this I would expect the encrypted data for these to be at least 6 octets longer than for the SBBT-002C, to be able to contain the ids and states for the additional three buttons. Similar to the unencrypted data length difference of these devices.

• You could remove the encrypted decoders and remove the option from the docs, and we'd have to wait until someone else might supply some encrypted sample data.
• You could use the app just for some brief encrypted samples. Then we'd be certain of the encrypted broadcasts and required decoders.
• You could adjust the encrypted decoders with the above mentioned theoretical additional 6 octets and adjust the cipher length and ctr and mic starting indexes accordingly, and also add some theoretical test cases.

What do you say? 😉

ADDENDUM: As the decoders are basically identical for the US and EU version of these 4 buttons switches I would also suggest to combine them into on single decoder, by
• shortening the model condition for the name to `"name", "index", 0, "SBBT-", but then also requiring an additional name length condition, as not to get into conflict with the SBBT-002C decoder. As I have yet to see any full sample broadcasts including the names for SBBT-004CEU, SBBT-004CUS as well as for the SBBT-002C, which I assume does also broadcast the name with the last two octets of its MAC address, I am not totally sure how this length definition would need to finally look like. Could you here also chyange the test cases with the actual full name being broadcast, replacing the MAC part with …EEFF instead of your personal device's specific address?
• the model_id could then be SBBT-004CEU/US
• The two docs should stay, so that users in each region will easily find their device in the compatible device's list, with the Model Id heading stating the individual model name, but both linking the the same, universally called SBBT_004CEU_US_json.h decoder

This would save a lot of space by not using duplicate decoders, with a universal model_id in the published MQTT messages not really being an issue for owners of either US or EU models. With the decoders running on ESP32 space is always a concern, and the space saved will be better used for addition totally different decoders in the future.

You might want to have a look at the IBS_THBP01B_json.h decoder, which decoders 4+ different Inkbird devices, all with their own docs, but using just the single decoder.

Lars added 7 commits September 18, 2024 16:26
…tation

- Added new device definition for ShellyBLU Switch4 (SBBT-004CEU/US) in `src/devices/SBBT_004CEU_US_json.h`.
- Updated `docs/devices/SBBT-004CEU.md` and `docs/devices/SBBT-004CUS.md` to indicate support for unencrypted devices only.
- Removed obsolete device definitions: `SBBT_004CEU_ENCR_json.h`, `SBBT_004CUS_ENCR_json.h`, `SBBT_004CEU_json.h`, and `SBBT_004CUS_json.h`.
- Updated `src/devices.h` and `src/decoder.h` to reflect the new device support and clean up unused includes.
@larswnd
Copy link
Contributor Author

larswnd commented Sep 18, 2024

I think we have to skip encryption for now, just guessing the encryption feels not right. I tried the app, but I do not own anything from Shelly that can act as Gateway...

I combined them into one.
My SBBT-002C does not have a MAC in its name, thats where my confusion came from 😄

hortening the model condition for the name to `"name", "index", 0, "SBBT-", but then also requiring an additional name length condition, as not to get into conflict with the SBBT-002C decoder.
There is an check for the servicedata length within the condition. So there shouldn't be a problem.

Could you here also chyange the test cases with the actual full name being broadcast, replacing the MAC part with …EEFF instead of your personal device's specific address?
Done.

@larswnd
Copy link
Contributor Author

larswnd commented Sep 18, 2024

I tested my Shelly Switch4 with the new decoder. It works great!

The Switch behaves a bit different than the SBBT-002C. The Single Button one does not send the 0xFE Event (Button hold), although it is mentioned in the docs. The Switch4 does so. But that has nothing to do with the decoder.

@DigiH
Copy link
Member

DigiH commented Sep 19, 2024

Thanks for the changes @larswnd

Please see my above review with remaining small amendments, then this should be all ready to merge.

@larswnd
Copy link
Contributor Author

larswnd commented Sep 20, 2024

Hello @DigiH
I thought I adjusted all of them. Did I miss some?

@DigiH
Copy link
Member

DigiH commented Sep 20, 2024

I thought I adjusted all of them. Did I miss some?

Hi @larswnd

Can you see my review comments within the code above? You might need to be logged in with a browser.

It's only the correct unified decoder file name in the docs link, and putting the 'unencrypted only' info into the Encrypted row of the docs table.

@DigiH
Copy link
Member

DigiH commented Sep 26, 2024

@larswnd

If you can't see the review comments, do you want me to make the small documentation changes so this can be merged?

@larswnd
Copy link
Contributor Author

larswnd commented Sep 26, 2024

I am sorry, I was travelling.
I do not see them (Logged In with Chrome on Mac).
But I know what to change, I will do it tomorrow

@DigiH
Copy link
Member

DigiH commented Sep 26, 2024

That's strange, but here is a screenshot :) no apology needed, get some rest from your travels first.

Screenshot 2024-09-26 at 19 35 48

@DigiH DigiH merged commit 6087438 into theengs:development Oct 15, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants