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 BLE Communication #96

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add BLE Communication #96

wants to merge 3 commits into from

Conversation

Kozova1
Copy link

@Kozova1 Kozova1 commented Apr 8, 2023

This PR adds BLE communication to the LucidGloves firmware.
Opengloves doesn't support it yet, so the only way to test it ATM is to use some kind of BLE explorer app on your phone.

I verified that this works and sends the data as expected on my phone.

@Kozova1
Copy link
Author

Kozova1 commented Apr 8, 2023

Currently has a limitation: Notify doesn't work with the current protocol, since it seems to be limited to about 20 bytes of payload. Maybe a more compact protocol can be used for BLE (maybe sending it as packed binary data?), or even just have a notify characteristic that tells the driver to read the "real" characteristic.

@Kozova1
Copy link
Author

Kozova1 commented Apr 8, 2023

It might be doable if we send the flexion values as shorts (16bit), and since they're 0-4095 always we can also use the upper 4 bits of the short for various booleans. This way we have exactly enough space, with maybe a few single bits left over. I'd prefer not to do this because this makes the implementation way harder on the decoder, and doesn't leave any room to expand in the future.

What I think should be done instead is to add a "data updated" characteristic, that simply notifies when the glove's data was changed, and then opengloves will need to know that when it receives that, it should read the full glove data.

@@ -25,6 +27,9 @@ void setup() {

void loop() {
if (comm->isOpen()){
#if COMMUNICATION == COMM_BLE
BLE.poll();
#endif
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is here because BLE needs polling, otherwise connections will fail.

I don't really like the way I did it here, but I'm not sure whether I should add a poll-like method to ICommunication or just keep it this way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is BLE defined?
and yeah, probably best to add a new method to ICommunication, but let's see what Lucas says.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a global provided by the ArduinoBLE library.

Copy link
Member

@danwillm danwillm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mainly work on the driver side but had a few questions (do let me know if you need any help with the implementation on the driver side), this all looks pretty good to me.

if (input == NULL) {
return false;
}
strcpy(input, reinterpret_cast<const char*>(m_rx.value()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an std::string? ideally use .c_str() if so, or if it's an arduino string iirc there's a .toCharArray - might that be helpful?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems to return a uint8_t *

Comment on lines +11 to +13
#define BLE_SERVICE_UUID "090b69e6-d509-4c32-8b20-5f8b947b253a"
#define BLE_TX_UUID "0eac19b9-5441-40bc-afca-cffb4380714d"
#define BLE_RX_UUID "54cd1674-a8aa-4b15-ac74-361b0084877f"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with the BLE spec so feel free to correct me - do these identifiers have to be unique between devices? (so no two gloves can have the same uuid?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so - I believe these just describe the "service" - in this case, the "serial" interface.

Comment on lines 33 to 35
int initialFlexion[5] = { 0 };
char* encoded = encode(initialFlexion, 0, 0, false, false, false, false, false, false, false, false);
m_tx.writeValue(encoded, strlen(encoded));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might not be, but it's better to initialize these to have well defined values instead of possibly causing undefined behavior

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, the array should probably 10 long, otherwise if splay is enabled this will cause reading beyond the end of the array. Will fix soon.

@Kozova1
Copy link
Author

Kozova1 commented Apr 21, 2023

Unfortunately, due to private reasons I won't be able to work on the opengloves side

#define COMM_BTSERIAL 1
#define COMM_BLE 2

#define BLE_SERVICE_UUID "090b69e6-d509-4c32-8b20-5f8b947b253a"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend using standard chars for uart: https://infocenter.nordicsemi.com/index.jsp?topic=%2Fcom.nordic.infocenter.sdk5.v13.0.0%2Fble_sdk_app_nus_eval.html
It is widely supported, you can download NRF connect for your phone, I use it quite a lot in my project

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.

3 participants