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

Endianness in addresses and UUIDs #86

Closed
lknop opened this issue Jul 18, 2020 · 14 comments
Closed

Endianness in addresses and UUIDs #86

lknop opened this issue Jul 18, 2020 · 14 comments

Comments

@lknop
Copy link
Contributor

lknop commented Jul 18, 2020

Since I am basing some functionalities on peer addresses and beacon UUIDs, I am struggling with incompatibilities of NimBLE against the older stack (and other vendors).

It seems all the addresses are reversed, which is not a huge issue per se since they are internally stored as 6-byte arrays, but perhaps this could be mentioned in the Breaking API changes document.

A larger problem, however, is connected with the UUIDs (so far I only noticed a problem with beacon uuids, service and characteristic ones work correctly). Example output from the BLE_Beacon_Scanner.ino:

Scan done!
Found an iBeacon!
iBeacon Frame
             ID: 004C Major: 1 Minor: 1 UUID: f31a5871-9179-2a97-d84d-f63cc5dc5fc7 Power: -56

However, the broadcast was configured with the UUID in reverse order:
#define BEACON_UUID "c75fdcc5-3cf6-4dd8-972a-799171581af3"

Other tools such as nRF Connect report the correct UUID:
beacon_scaled

And the scanner based on Bluedroid also discovers it as c75fdcc5-3cf6-4dd8-972a-799171581af3, not the other way round.

The question is where it should be solved - the obvious location would be void NimBLEBeacon::setData(const std::string &data) where the data is set directly with memcpy whereas other methods do perform ENDIAN_CHANGE_U16 on other fields. Although proximityUUID is never reversed, and it most probably should be.

However, I am wondering whether this could not be solved globally, any migration from bluedroid will need conversion of addresses, and even if that is relatively simple, keeping the data in little endian arrays is cumbersome in any debug display or string conversion.

As a side note, it seems strange to me that I have found only a single mention of the endianness issue in NimBLE docs.

lknop added a commit to lknop/NimBLE-Arduino that referenced this issue Jul 18, 2020
lknop added a commit to lknop/NimBLE-Arduino that referenced this issue Jul 18, 2020
@lknop
Copy link
Contributor Author

lknop commented Jul 18, 2020

It seems that reversing is already supported in the getter for the proximityUUID, created #87 to use it.

However, it still does not fix the general issue:

NimBLEUUID("c75fdcc5-3cf6-4dd8-972a-799171581af3") will not be equal to the proximityUUID of a discovered BLE beacon with the same uuid.

EDIT: found a workaround I can get away with, that only depends on the #87 getter fix:

NimBLEBeacon discoveredBeacon = NimBLEBeacon();
discoveredBeacon.setData(strManufacturerData);
NimBLEBeacon compareBeacon = NimBLEBeacon();
compareBeacon.setProximityUUID(NimBLEUUID("c75fdcc5-3cf6-4dd8-972a-799171581af3"));
if (discoveredBeacon.getProximityUUID() == compareBeacon.getProximityUUID()) {
    // the UUIDs match 
}

@h2zero
Copy link
Owner

h2zero commented Jul 18, 2020

Hi @lknop, thanks for pointing this out. Admittedly I have not tested the beacon code enough so this has gone unchecked for quite a while.

If I am understanding the issue correctly, when you log the beacon uuid it's showing it in little endian format, which is obviously not what we want to see on the console, but when you apply the proposed fix the uuid comparison fails?

My question then is, does the comparison of NimBLEUUID("c75fdcc5-3cf6-4dd8-972a-799171581af3") == discoveredBeacon.getProximityUUID() work without #87 fix? I think it would since the created uuid is also reversed.

However that means the uuid is still reversed in the log, which could be dealt with separately.

You also mentioned the addresses, are you seeing them reversed as well? If so can you provide more detail?

@h2zero
Copy link
Owner

h2zero commented Jul 18, 2020

I was just able to test this and I am unable to reproduce the issue, could you provide some example code for the beacon and scanner?

@lknop
Copy link
Contributor Author

lknop commented Jul 18, 2020

I advertise the beacon from hcitool:

sudo hcitool cmd 0x08 0x0008 1E 02 01 1A 1A FF 4C 00 02 15 C7 5F DC C5 3C F6 4D D8 97 2A 79 91 71 58 1A F3 00 01 00 01 C8
sudo hcitool cmd 0x08 0x000A 01

The console output I pasted in the original description is from the stock BLE_Beacon_Scanner.ino

I also test it via Beacon simulator android app which is configured like this:
Screenshot_20200718-165129_Beacon Simulator

And outputs the following in the console:

iBeacon Frame
             ID: 004C Major: 0 Minor: 0 UUID: 4e93fb73-b9f3-949a-1548-62081e5d5b7f Power: -65

My question then is, does the comparison of NimBLEUUID("c75fdcc5-3cf6-4dd8-972a-799171581af3") == discoveredBeacon.getProximityUUID() work without #87 fix? I think it would since the created uuid is also reversed.

Yes in fact it does. I have first concentrated on the invalid logging and only after forcing the reversal did I discover the comparison failing.

@lknop
Copy link
Contributor Author

lknop commented Jul 18, 2020

Regarding the addresses, when doing the rewrite of the onConnect callback, to get the same address representation I had to reverse the byte order. It seems that esp_bd_addr_t is big-endian and ble_addr_t.val is little-endian.

Compare the two snippets from the onConnect callback:

  1. bluedroid version
void onConnect(BLEServer *pServer, esp_ble_gatts_cb_param_t *param)
{
    sprintf(
      remoteAddress,
      "%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
      param->connect.remote_bda[0],
      param->connect.remote_bda[1],
      param->connect.remote_bda[2],
      param->connect.remote_bda[3],
      param->connect.remote_bda[4],
      param->connect.remote_bda[5]
    );
}
  1. nimble version:
void onConnect(NimBLEServer *pServer, ble_gap_conn_desc *param)
{    
  sprintf(
      remoteAddress,
      "%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
      param->peer_ota_addr.val[5],
      param->peer_ota_addr.val[4],
      param->peer_ota_addr.val[3],
      param->peer_ota_addr.val[2],
      param->peer_ota_addr.val[1],
      param->peer_ota_addr.val[0]
    );
}

@h2zero
Copy link
Owner

h2zero commented Jul 18, 2020

Thanks, I was able to reproduce the issue using the phone app as the beacon. I also used the BLE_Beacon_Scanner.ino with NimBLE and Bluedroid and the results are the same, both produce the uuid reversed.

My question then is, does the comparison of NimBLEUUID("c75fdcc5-3cf6-4dd8-972a-799171581af3") == discoveredBeacon.getProximityUUID() work without #87 fix? I think it would since the created uuid is also reversed.

Yes in fact it does. I have first concentrated on the invalid logging and only after forcing the reversal did I discover the comparison failing.

In this case we need to solve this in a way that satisfies comparisons and logging. I think the place to look is in the beacon code, since the issue happens with both NimBLE and Bluedroid and only with beacons, global changes elsewhere may cause more problems.

As for the address in the callback, yes it is reversed there as that is what NimBLE gives us, however you could use the NimBLEAddress constructor to make things easier depending on your use for it:

NimBLEAddress peerAddr(param->peer_ota_addr);

@lknop
Copy link
Contributor Author

lknop commented Jul 18, 2020

Thanks, I was able to reproduce the issue using the phone app as the beacon. I also used the BLE_Beacon_Scanner.ino with NimBLE and Bluedroid and the results are the same, both produce the uuid reversed.

I have also been testing this independently and you are right - this is a bug but it is present in bluedroid as well. I don't know why I thought I had tested it previously since I only got to uuid filtering after migration to nimble.

In this case we need to solve this in a way that satisfies comparisons and logging. I think the place to look is in the beacon code, since the issue happens with both NimBLE and Bluedroid and only with beacons, global changes elsewhere may cause more problems.

I concur.

As for the address in the callback, yes it is reversed there as that is what NimBLE gives us, however you could use the NimBLEAddress constructor to make things easier depending on your use for it:

`NimBLEAddress peerAddr(param->peer_ota_addr);

That's a very handy convenience class, didn't see it before, thanks.

@h2zero
Copy link
Owner

h2zero commented Jul 18, 2020

@lknop I just pushed a branch bugfix/beacon-uuid that i think solves the issue for both beacon and scanner as well as phone apps. If you could give it a try and let me know it would be appreciated.

@chegewara
Copy link

I dont see changes, but only change you have to do is receiver/scanner side, not the sender.
Its due to endianess.

@lknop
Copy link
Contributor Author

lknop commented Jul 18, 2020

@h2zero it sort of does, but the effect is similar to my change, the comparison with manually contructed NimBLEUUID is false, one has to use the same workaround I did before - create another NimBLEBeacon and compare their proximityuuids:

if (discoveredBeacon.getProximityUUID() == compareBeacon.getProximityUUID()) {
    // the UUIDs match 
}

The reason for this is that the following code:

Serial.println(NimBLEUUID("c75fdcc5-3cf6-4dd8-972a-799171581af3").toString().c_str());

results in

f31a5871-9179-2a97-d84d-f63cc5dc5fc7

I have also figured out why I did not notice the issue before, when using bluedroid I was not using BLEBeacon but printing the whole payload from the advertisement which is not reversed: "payload":"02011a1aff4c000215c75fdcc53cf64dd8972a799171581af300010001c8"

@h2zero
Copy link
Owner

h2zero commented Jul 18, 2020

I dont see changes, but only change you have to do is receiver/scanner side, not the sender.
Its due to endianess.

@chegewara The reason I changed the sender is that for some reason when we get the uuid from the esp32 the endianess of the uuid is opposite of the phone apps. Not sure which is correct so this is just testing for now. For what it's worth bluedroid does the same thing, any thoughts on why it's different from phone apps?

the comparison with manually contructed NimBLEUUID is false
The reason for this is that the following code:
Serial.println(NimBLEUUID("c75fdcc5-3cf6-4dd8-972a-799171581af3").toString().c_str());
results in
f31a5871-9179-2a97-d84d-f63cc5dc5fc7

@lknop I do not see this in my testing, I wonder what you're doing different than me?

@h2zero
Copy link
Owner

h2zero commented Jul 18, 2020

I just confirmed with a different app that the beacon/sender needs to be reversed as well as the scanner receiver.

The phone would not find the esp32 beacon until I reversed the uuid. So the scanner needs to reverse it as well to correct the data for our purposes.

With #87 applied and changing NimBLEBeacon::setProximityUUID to:

void NimBLEBeacon::setProximityUUID(const NimBLEUUID &uuid) {
    NimBLEUUID temp_uuid = uuid;
    temp_uuid.to128();
    std::reverse_copy(temp_uuid.getNative()->u128.value,
                      temp_uuid.getNative()->u128.value + 16,
                      m_beaconData.proximityUUID);
} // setProximityUUID

Everything seems to work as expected in my testing. However I'm confused why @lknop is getting reversed UUIDs from construction...

@lknop
Copy link
Contributor Author

lknop commented Jul 18, 2020

However I'm confused why @lknop is getting reversed UUIDs from construction...

I had an uncommited change from my previous experiments 🤦

I cleaned up my working copy, added the commit you described above to my PR and I will be testing it in a few hours.

@h2zero
Copy link
Owner

h2zero commented Jul 18, 2020

Lol that would explain it, thought that might have been the case.

Hopefully everything works now.

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

3 participants