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 data encryption support according to the bthome protocol #3

Closed
wants to merge 0 commits into from

Conversation

junchao98
Copy link

hello everyone,

This submission I tested normal in the following cases

  1. Encryption of individual sensor data
  2. Encrypting multiple sensor data
  3. No encryption

@junchao98 junchao98 mentioned this pull request Apr 15, 2023
@stumpylog
Copy link
Owner

Awesome, thanks for the contribution. I'll have to pull my sensor package out of its case to test this, so it may take a few more days before I can merge it.

Just glancing through, it makes sense. I do see what looks like some formatting diffs. Could you run the clang formatter (there's format.sh for it)? Thanks!

@Chreece
Copy link

Chreece commented Apr 28, 2023

@junchao98 if you have time could you port your code to the general ESP32 example to finally have one with encryption?
https://github.com/Chreece/BTHomeV2-ESP32-example
(my programming skills are unfortunately basic)

@junchao98
Copy link
Author

@junchao98 if you have time could you port your code to the general ESP32 example to finally have one with encryption? https://github.com/Chreece/BTHomeV2-ESP32-example (my programming skills are unfortunately basic)

No problem, I will submit the code in the near future

@countrysideboy
Copy link

countrysideboy commented May 9, 2023

@junchao98 if you have time could you port your code to the general ESP32 example to finally have one with encryption? https://github.com/Chreece/BTHomeV2-ESP32-example (my programming skills are unfortunately basic)

No problem, I will submit the code in the near future

Great! Thanks.

@stumpylog stumpylog linked an issue May 10, 2023 that may be closed by this pull request
@stumpylog
Copy link
Owner

stumpylog commented May 10, 2023

Testing this change out, I get an abort here:

ESP_ERROR_CHECK failed: esp_err_t 0x102 (ESP_ERR_INVALID_ARG) at 0x40090fb8
file: "./main/task_ble.cpp" line 112
func: void task_ble_entry(void*)
expression: esp_ble_gap_config_adv_data_raw(&advertData[0], dataLength)

Presumably, this is due to the advertisement being 35 bytes, more than the 31 allowed. It should probably check that and warn at least before aborting.

Edit: In fact, it would probably be good to support encryption, but no name, as the encryption adds so many extra bytes

@stumpylog
Copy link
Owner

Once I changed up the naming to be short enough (exactly 31 bytes), Home Assistant did pick it up as encryption and the bind key worked.

Just a small tweak to try and make it a little more friendly of an error I think, and I can merge it in

@junchao98
Copy link
Author

Once I changed up the naming to be short enough (exactly 31 bytes), Home Assistant did pick it up as encryption and the bind key worked.

Just a small tweak to try and make it a little more friendly of an error I think, and I can merge it in

Yes, the esp-idf api has a requirement for maximum data length.

@stumpylog
Copy link
Owner

I know this looks closed, that's what I get for trying to manage too many remotes. I tweaked the changes a little so it won't abort if too long, and merged in the commits.

@countrysideboy
Copy link

@junchao98 if you have time could you port your code to the general ESP32 example to finally have one with encryption? https://github.com/Chreece/BTHomeV2-ESP32-example (my programming skills are unfortunately basic)

No problem, I will submit the code in the near future

I have ported your code to the BTHomeV2-ESP32-example project, thanks for your hard working.

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.

Encryption with BindKey?
4 participants