taks / esp32-nimble

A wrapper for the ESP32 NimBLE Bluetooth stack.
Apache License 2.0
118 stars 35 forks source link

Creating a new BLEAddress is confusing because it reverses the bytes. #135

Closed ChocolateLoverRaj closed 3 months ago

ChocolateLoverRaj commented 3 months ago

https://github.com/taks/esp32-nimble/blob/080cf477cf9cfcb56dc795e14e9fe51a6d456cb1/src/ble_address.rs#L28

When you use new() to create a BLEAddress and then use val() on it, you should get the same address as you had originally. Idk why the new() function reverses the bytes. If the bytes need to be reversed, they should be reversed before they are given to the new() function. The from_str function should reverse the bytes itself.

Also, it would be useful if there was a function that converted a string address to [u8; 6] that can be used outside of a ESP32, like on a normal computer. For the project I am doing, I need to send a [u8; 6] from a normal computer to a ESP32.

And the from_str function should have comments explaining how it works and an example.

I can make a PR for these things, but I wanted to discuss this with you first since this involves breaking changes.

taks commented 3 months ago

I think it is better to pass the bytes array in the order in which they appear in the address string, as shown below.

let addr = BLEAddress::new([0, 1, 2, 3, 4, 5], BLEAddressType::Public);
::log::info!("{}", a); // => 00:01:02:03:04:05

(I checked other libraries. They are also passing bytes array in this order.)

However, it is not good if the argument of new() is not same as the return value of val(). How about changing val() to return the inverted value?

ChocolateLoverRaj commented 3 months ago

How about changing val() to return the inverted value?

That would be better, but still have the extra steps of reversing the address.

I have an idea, what about having functions called new_from_be and new_from_le, and val_be and val_le? The new_from_be function will reverse the value. new_from_le will not reverse the value because it will already be in little endian format. val_be will reverse the value since it is stored as LE. val_le will not reverse the value because it is already stored as LE.

To transition, you could make the new function an alias for new_be and the val functions an alias for val_le. That way, there isn't a breaking change, and if you want you could eventually deprecate it.

Also, I think it would be good to directly parse a string like "00:01:02:03:04:05" into an array [5, 4, 3, 2, 1], without specifying an address type. And if this function could be called from a non-esp32 computer that would be convenient.

taks commented 3 months ago

Thanks.

I have an idea, what about having functions called new_from_be and new_from_le, and val_be and val_le? The new_from_be function will reverse the value. new_from_le will not reverse the value because it will already be in little endian format. val_be will reverse the value since it is stored as LE. val_le will not reverse the value because it is already stored as LE.

Your idea is good. The names of those functions, from_be_bytes, from_le_bytes, as_be_bytes and as_le_bytes, are more rust-like.

Could you create a pull request?