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

Arduino Nano 33 IoT large transfer fix (MCU hang bug) #316

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

coderand
Copy link

@coderand coderand commented Aug 5, 2023

uint16_t handle = *(uint16_t*)data

hangs Arduino Nano 33 IoT in cases when data pointer is unaligned. This happens when receiving large packets of BLEStringCharacteristic.

Cortex®-M0 does not support unaligned memory access.

This doesn't not happen when setting such characteristics from iPhone (iOS) or Android devices, but happens 100% of the time on PC using either Bleak (Python) or QT6 libraries.

In those cases data pointer at the time of this call is not two bytes (sizeof(uint16_t)) aligned and dereferencing such pointer hangs the MCU.

To resolve the issue we copy that uint16_t handle byte by byte without monkeying with ORs, Shifts or other binary operations to avoid endianness issues on other platforms.

uint16_t handle = *(uint16_t*)data

hangs Arduino Nano 33 IoT in cases when data pointer is unaligned. This happens when receiving large packets of BLEStringCharacteristic.

This doesn't not happen when setting such characteristics from iPhone (iOS) or Android devices, but happens 100% of the time on PC using either Bleak (Python) or QT6 libraries.

In those cases data pointer at the time of this call is not two bytes (sizeof(uint16_t)) aligned and dereferencing such pointer hangs the MCU.

To resolve the issue we copy that uint16_t handle byte by byte without monkeying with ORs, Shifts or other binary operations to avoid endianness issues on other platforms.
@CLAassistant
Copy link

CLAassistant commented Aug 5, 2023

CLA assistant check
All committers have signed the CLA.

@coderand
Copy link
Author

coderand commented Aug 5, 2023

Alternatively, instead of uint16_t handle = *(uint16_t*)data;
it could be:
uint16_t handle;
memcpy(&handle, data, sizeof(handle));
that's a bit more consistent with the rest of the coding style there.

Apart from that, it looks like there could be a few more potential bugs of this sort but I have no way to test or confirm those. The most obvious one would probably be in ATTClass::readOrReadBlobReq method, where uint8_t data[] is dereferenced as *(uint16_t *)data as well.

@coderand
Copy link
Author

coderand commented Aug 5, 2023

This mainly happens when setting long values such as "012345678901234567890123456789" on Windows 10 using pretty much any BLE tool or API. Writing characteristic with "Microsoft LE Expolorer", using Python/Bleak or Qt6

The Arduino NANO 33 IoT Sketch is:

#include <ArduinoBLE.h>
#include <utility/ATT.h>
#include <utility/GATT.h>

BLEService testService("180B");
BLEStringCharacteristic testCharacteristic("00000003-0000-1000-8000-00805f9b34fb", BLEWrite | BLERead | BLENotify, 185);

void setup() {
  Serial.begin(9600);
  while (!Serial);

  ATT.setMaxMtu(185);

  if (!BLE.begin()) {
    while (1);
  }

  GATT.setDeviceName("IoT-Hang");
  BLE.setLocalName("IoT-Hang");
  BLE.setDeviceName("IoT-Hang");
  BLE.setAdvertisedService(testService);
  testService.addCharacteristic(testCharacteristic);
  BLE.addService(testService);
  BLE.advertise();
}

void loop() {
  BLEDevice central = BLE.central();

  if (central && central.connected()) {
    if (testCharacteristic.written()) {
      Serial.println(testCharacteristic.value());
    }
  }
}

@per1234 per1234 added type: imperfection Perceived defect in any part of project topic: code Related to content of the project itself labels Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants