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

start_advertising interval does not accept the minimum 20ms value and may not range check correctly #2930

Closed
kevinjwalters opened this issue May 19, 2020 · 6 comments · Fixed by #3964

Comments

@kevinjwalters
Copy link

kevinjwalters commented May 19, 2020

I was reading https://developer.apple.com/accessories/Accessory-Design-Guidelines.pdf and page 126, section 35.5 recommends "an initial 30s of 20ms intervals" for advertising if I'm understanding this correctly.

I tried it on CircuitPython and it didn't behave. I've just run it again against 5.3.0 with 20200519 libs.

Adafruit CircuitPython 5.3.0 on 2020-04-29; Adafruit Circuit Playground Bluefruit with nRF52840
>>>
>>> from adafruit_ble import BLERadio
>>> from adafruit_ble.advertising.adafruit import AdafruitColor
>>> ca = AdafruitColor()
>>> ca.color = 0xee0000
>>> ca
Advertisement(data=b"\x0a\xff\x22\x08\x06\x00\x00\x00\x00\xee\x00")
>>> ble = BLERadio ()
>>> ble.start_advertising(ca, interval=0.020)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "adafruit_ble/__init__.py", line 185, in start_advertising
_bleio.BluetoothError: Unknown soft device error: 0007

I had a brief discussion with @jepler on discord about this and we both assumed it was an unfortunate FP-ism. I had a glance at the code and it looks like it might unfortunately reject via an informational exception a 0.020 value because of the 30-bit fp conversion and then comparison of < 0.020f. The nice exception doesn't happen, I tried a few values to work out what's going on.

>>> ble.start_advertising(ca, interval=0.019)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "adafruit_ble/__init__.py", line 185, in start_advertising
_bleio.BluetoothError: Unknown soft device error: 0007
>>> ble.start_advertising(ca, interval=0.001)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "adafruit_ble/__init__.py", line 185, in start_advertising
ValueError: interval must be in range 0.0020-10.24

Are these pair (https://github.com/adafruit/circuitpython/blob/master/shared-bindings/_bleio/Adapter.c#L36-L37) supposed to read 0.020 (20ms) rather than 0.0020 (2ms)?

The code is also not doing an explicit conversion to integer so gets truncation (round towards zero) semantics and judging by a test of this code that will also punt a 30bit 0.020 value into out of range territory:

#define SEC_TO_UNITS(TIME, RESOLUTION) (((TIME) * 1000000) / (RESOLUTION))
#define UNIT_0_625_MS (625)

int main(int argc, char *argv[]) {
  float f1 = 0.020f;
  /* 0.020 into Schmidt FP tool, zero lower two digitals, take "actual" value - this is way beyond 32bit prec. */
  float f2 = 0.01999999582767486572265625f;
  float f3 = 0.02001;
  int i1, i2, i3;

  if (f2 < f1) {
    printf("TOO SMALL\n");
  };

  i1 = SEC_TO_UNITS(f1, UNIT_0_625_MS);
  i2 = SEC_TO_UNITS(f2, UNIT_0_625_MS);
  i3 = SEC_TO_UNITS(f3, UNIT_0_625_MS);

  fprintf(stdout, "%f is %d and %f2 is %d and %f2 is %d\n", f1, i1, f2, i2, f3, i3);
}

Output is:

TOO SMALL
0.020000 is 32 and 0.0200002 is 31 and 0.0200102 is 32

The middle one is actually the 0.020 with MicroPython's 30bit conversion applied and comes out as 31 which is below minimum.

For anyone else stumbling across this issue, the value below works as a workaround that can be left in place after any fixes:

>>> ble.start_advertising(ca, interval=0.02001)
@kevinjwalters
Copy link
Author

kevinjwalters commented May 20, 2020

One other thing I'm puzzling over is why the flags (0x01) appear if I run dir() on an Advertisement object.

Adafruit CircuitPython 5.3.0 on 2020-04-29; Adafruit Circuit Playground Bluefruit with nRF52840
>>> from adafruit_ble.advertising.adafruit import AdafruitColor
>>> ca = AdafruitColor()
>>> ca.color = 0x112233
>>> ca
Advertisement(data=b"\x0a\xff\x22\x08\x06\x00\x00\x33\x22\x11\x00")
>>> dir(ca)
['__bytes__', '__class__', '__dict__', '__init__', '__len__', '__module__', '__qualname__', '__repr__', '__str__', 'address', 'connectable', 'matches', 'rssi', 'scan_response', 'tx_power', 'complete_name', 'from_entry', 'prefix', 'data_dict', 'flags', 'mutable', 'short_name', 'appearance', '_rssi', 'manufacturer_data', 'color']
>>> ca
Advertisement(data=b"\x02\x01\x00\x0a\xff\x22\x08\x06\x00\x00\x33\x22\x11\x00")

Note 020100 now sits on the front. This must be related to some lazy initialization but it's not clear to me why this would have an effect on its byte representation. This feels either fragile or wrong...

@dhalbert
Copy link
Collaborator

@tannewt tannewt added this to the Long term milestone Jun 22, 2020
This was referenced Dec 18, 2020
@kevinjwalters
Copy link
Author

There's something not quite right on 6.0.1, the value of 0.02001 trips the range checking logic:

Adafruit CircuitPython 6.0.1 on 2020-12-28; Adafruit CLUE nRF52840 Express with nRF52840
>>> from adafruit_ble import BLERadio
>>> from adafruit_ble.advertising.adafruit import AdafruitColor
>>> ca = AdafruitColor()
>>> ca.color = 0x4b4c46
>>> ca
Advertisement(data=b"\x0a\xff\x22\x08\x06\x00\x00\x46\x4c\x4b\x00")
>>> ble = BLERadio ()
>>> ble.start_advertising(ca, interval=0.020)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "adafruit_ble/__init__.py", line 212, in start_advertising
ValueError: interval must be in range 0.02001-10.24
>>> ble.start_advertising(ca, interval=0.02001)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "adafruit_ble/__init__.py", line 212, in start_advertising
ValueError: interval must be in range 0.02001-10.24
>>> ble.start_advertising(ca, interval=0.0200100008)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "adafruit_ble/__init__.py", line 212, in start_advertising
ValueError: interval must be in range 0.02001-10.24
>>> ble.start_advertising(ca, interval=0.0200100009)
>>> ble.stop_advertising()

@dhalbert
Copy link
Collaborator

dhalbert commented Jan 2, 2021

The floating-point conversations are apparently different for the C compiler and the CircuitPython/MicroPython converter. So for now, could you just use a larger value? I will try to make this more consistent.

@kevinjwalters
Copy link
Author

I'm not actively using that value in new code but I mentioned 0.02001 as a workaround in this ticket that's been around since last May. That value is also in the code in Adafruit Learn: CLUE Rock, Paper, Scissors Game using Bluetooth but that also doesn't work on 6 so far due to #3731

@kevinjwalters
Copy link
Author

kevinjwalters commented Apr 17, 2021

The dir() issue mentioned in comments is probably related partly to micropython#4546 / #4171

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants