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

Advertised device payload not copied? #55

Closed
Jeroen88 opened this issue May 20, 2020 · 17 comments
Closed

Advertised device payload not copied? #55

Jeroen88 opened this issue May 20, 2020 · 17 comments

Comments

@Jeroen88
Copy link

Is there any good reason why payload is not copied into the advertised device member variable std::string m_payload? Now only the pointer returned from the stack is saved, see here.

@h2zero
Copy link
Owner

h2zero commented May 20, 2020

I think the simple answer would be to save resources. 31 bytes + std::string overhead per device found. It’s also what was in the original library so...

@Jeroen88
Copy link
Author

But manufacturer data is copied, that is inconsistent, isn't it?

Can I make a PR to copy it? Or will the payload of all scanned devices be accessible through the scan object? I doubt it...

@chegewara
Copy link

Maybe this time would be better to left user option to copy payload and other advertising data, this wont use memory in case user is not interested some found devices, especially useful in dense environment.

I think low level stack is keeping only few most recent seen devices, at least in bluedroid.

@Jeroen88
Copy link
Author

@chegewara I see your point. @chegewara @h2zero this could also be set dynamically, like the auto pre discovery. I would opt for all or nothing, so store all data in the advertised device including the payload, or nothing at all. Also I would opt for changing payload into a std::string instead of a uint8_t *, because all other advertised device data are std::string's. For consistency.

@chegewara
Copy link

Also I would opt for changing payload into a std::string instead of a uint8_t *, because all other advertised device data are std::string's.

I dont agree. Payload is type of uint8* and cant be treated as string. I know you may have not printable chars in string, even 0x0 in middle of it, and maybe even it is C++ acceptable, but this is not good idea. If you want to go that way you can replace all std:string with arduino String type.

@Jeroen88
Copy link
Author

std::string is (mis-) used throughout this library and the original library

@chegewara
Copy link

chegewara commented May 20, 2020

Yes, i know. Maybe its time to fix it and use variable types that should be used? Just saying, if we know that something was wrong that does not mean it should be replicated. Lets try to make this library better than the other one.

@h2zero
Copy link
Owner

h2zero commented May 20, 2020

I agree with @chegewara on this. We already parse the data from the payload and store it anyway, no need for another copy. If the user wants to save it they can copy it to whatever container they want with the pointer.

Not sure why you’re using this anyway? Is there some data in the payload that isn’t available from the advertisedDevice object?

@h2zero
Copy link
Owner

h2zero commented May 20, 2020

Yes, i know. Maybe its time to fix it and use variable types that should be used? Just saying, if we know that something was wrong that does not mean it should be replicated. Lets try to make this library better than the other one.

I fear the PR flood gates just got smashed with this comment lol.

@Jeroen88
Copy link
Author

I fear the PR flood gates just got smashed with this comment lol.

Lol, time for a revolution and a contra revolution. Finally! :D

In #48 the pre revolution started already, but then it was a one day war only... ;)

We already parse the data from the payload and store it anyway

Nope, the fields are parsed here and the payload is set (pointer copied) just below.

Not sure why you’re using this anyway? Is there some data in the payload that isn’t available from the advertisedDevice object?

Short answer: yes, e.g.:

Address is a4:c1:38:5d:ef:16
Service data UUID is 0xfe95
0x30, 0x58, 0x5b, 0x05, 0x01, 0x16, 0xef, 0x5d, 0x38, 0xc1, 0xa4, 0x28, 0x01, 0x00, 
Payload is:
0x02, 0x01, 0x06, 0x15, 0x16, 0x95, 0xfe, 0x50, 0x20, 0xaa, 0x01, 0xb3, 0x58, 0x22, 0x39, 0x34, 0x2d, 0x58, 0x0d, 0x10, 0x04, 

I do not know what I want with it exactly. I am looking for a way to do a scan and filter out the devices that I support. One way is with getName(), but 2 of my 3 devices don't have a name. I could read the characteristic, but then I have to connect. I would like to determine one way or another what from the advertised data which devices I have support for. It must be generic enough to recognize all same devices, but specific enough to differentiate between e.g. two different devices of the same manufacturer. Any suggestions?

@Staars
Copy link
Contributor

Staars commented May 20, 2020

Any suggestions?

You simply have to parse a MiBeacon and make the decision based on the PID (the value after the frame_ctrl). In your example 0x5b 0x05 stands for the LYWSD03 and here you will have to connect and read the data (or you have the beacon key to decrypt the ADV).
Most of the other Xiaomi/Mijia-sensors should provide their data in the ADV, so you can parse it directly from the scan data.
This approach works for me.

Look here for further info: https://github.com/Magalex2x14/LYWSD03MMC-info

@Jeroen88
Copy link
Author

Jeroen88 commented May 20, 2020

@Staars Thank you Christian for directing me to that site again. I now almost know the structure of the service data for the MJ-HT-V1:

struct ServiceData {
  uint16_t frameCtrl;
  uint16_t deviceType;
  uint8_t frameCnt;
  uint8_t MACAddress[6];
  uint8_t variantType;
  uint16_t unknown;
  union {
    uint16_t temperature;
    uint16_t humidity;
    uint8_t battery;
    struct {
      uint16_t temperature;
      uint16_t humidity;
    } both;
  } variants;
};

Just 2 bytes to discover: do you know what the 2 bytes at position 12 and 13 mean?

Also still looking what the last bytes mean if byte 11 equals 0xdc.

Is this structure generic for every BLE peripheral or for Xiaomi only?

Do you happen to know if the LYWSD03 can be read without connection?

@Staars
Copy link
Contributor

Staars commented May 22, 2020

@Jeroen88 (a bit late ;)

Just 2 bytes to discover: do you know what the 2 bytes at position 12 and 13 mean?

You mean the last two bytes before the data?

It is this enum:
https://github.com/MiEcosystem/mijia_ble_common/blob/847ee73ca9ba496a91483d300a8bd6a7666a8c0c/mible_beacon.h#L28
and the the length in bytes.
So uint8_t variantType; uint16_t unknown;
must be changed to:
uint16_t type; uint8_t len;

Also still looking what the last bytes mean if byte 11 equals 0xdc.

I am not sure, but my best guess is: RSSI

Is this structure generic for every BLE peripheral or for Xiaomi only?

No, this is Xiaomi only, but BTW I am not aware of any other sensor families (temp/hum-stuff for homes) out there.

Do you happen to know if the LYWSD03 can be read without connection?
Yes, it can be done, but with many "if's" and everything is described in the link from my older post.
Basically:

  1. A fresh LYWSD03 will only send advertisements without sensor data ("unbinded" frame):
    30 58 5b 05 01 xx xx xx xx xx xx 28 01 00
  2. After pairing with your phone with the MiHome-App this will change to an "encrypted" frame (only about every 10 minutes!!).
  3. When you have sniffed the "bind_key", you can decrypt the data without a connection purely from the advertisement.

My personal opinion is, that I find it more convenient to connect to the LYWSD03 from time to time. I prefer to avoid the relatively complicated procedure to obtain the "bind_key". Another advantage of the connection-method is, that you can read the real battery voltage instead of the questionable battery-percentage-level.

@Jeroen88
Copy link
Author

Thx @Staars for the extensive answers!

It is this enum:

Going to check that link later!

must be changed to:

Are you sure that it is not the other way around, uint8_t type and uint16_t len?

I find it more convenient to connect to the LYWSD03

Yes I thin you are right, that is the easiest option.

@Staars
Copy link
Contributor

Staars commented May 23, 2020

Are you sure that it is not the other way around, uint8_t type and uint16_t len?

Pretty sure.
Maybe it is a bit confusing to take a look at the payload seeing (and then thinking) in single bytes, but in reality the type is a word and then the byte order is swapped. So for the MJ_HT_V1 the type is always 10xx which becomes xx 10 in the byte stream, followed by 1,2 or 4 for the length in bytes for the actual data (temp, hum, bat or temp+hum).
At least I got confused regularly 😁

@Jeroen88
Copy link
Author

Jeroen88 commented May 23, 2020

😆 thnx!

Pretty sure

I took a better look at the link, since it seems to be the "official" site I understand the pretty now...

@h2zero
Copy link
Owner

h2zero commented Feb 1, 2021

Closing this as recent commits have addressed some of this issue. Version 2.0.0 will address the rest.

@h2zero h2zero closed this as completed Feb 1, 2021
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

No branches or pull requests

4 participants