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

sys/stdio_nimble: add version note to README #21085

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

crasbe
Copy link
Contributor

@crasbe crasbe commented Dec 13, 2024

Contribution description

In the latest release of ble-serial, a new command line option --write-with-response was added and the default value is False.
(See Jakeler/ble-serial#109 and https://github.com/Jakeler/ble-serial/releases/tag/v3.0.0 ).
This will lead to the following error when trying to connect to the test/sys/shell_ble example or any application that uses the stdio_nimble module.

~/RIOTdev/RIOT/tests/sys/shell_ble$ ble-serial -d DA:ED:02:2A:35:CE -p /tmp/dev_riot_ble --write-uuid ccdd113f-40d5-4d68-86ac-a728dd82f4aa --read-uuid 35f28386-3070-4f3b-ba38-27507e991762
16:46:15.471 | INFO | linux_pty.py: Port endpoint created on /tmp/dev_riot_ble -> /dev/pts/1
16:46:15.471 | INFO | ble_client.py: Receiver set up
16:46:17.565 | INFO | ble_client.py: Trying to connect with DA:ED:02:2A:35:CE: tests/sys/shell/_ble
16:46:17.973 | WARNING | ble_client.py: Device DA:ED:02:2A:35:CE disconnected
16:46:17.974 | INFO | ble_client.py: Stopping Bluetooth event loop
16:46:19.446 | INFO | ble_client.py: Device DA:ED:02:2A:35:CE connected
16:46:19.446 | ERROR | main.py: No characteristic with ['write-without-response'] property found!
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/dist-packages/ble_serial/main.py", line 53, in _run
    await self.bt.setup_chars(args.write_uuid, args.read_uuid, args.mode, args.write_with_response)
  File "/usr/local/lib/python3.10/dist-packages/ble_serial/bluetooth/ble_client.py", line 44, in setup_chars
    self.write_char = self.find_char(write_uuid, write_cap)
  File "/usr/local/lib/python3.10/dist-packages/ble_serial/bluetooth/ble_client.py", line 84, in find_char
    assert len(results) > 0, \
AssertionError: No characteristic with ['write-without-response'] property found!
16:46:19.447 | WARNING | main.py: Shutdown initiated
16:46:19.447 | INFO | linux_pty.py: Serial reader and symlink removed
16:46:21.677 | WARNING | ble_client.py: Device DA:ED:02:2A:35:CE disconnected
16:46:21.677 | INFO | ble_client.py: Stopping Bluetooth event loop
16:46:21.678 | INFO | ble_client.py: Bluetooth disconnected
16:46:21.678 | INFO | main.py: Shutdown complete.

Using the new command line option makes the command work as expected:

~/RIOTdev/RIOT/tests/sys/shell_ble$ ble-serial -d DA:ED:02:2A:35:CE -p /tmp/dev_riot_ble --write-uuid ccdd113f-40d5-4d68-86ac-a728dd82f4aa --read-uuid 35f28386-3070-4f3b-ba38-27507e991762 --write-with-response
16:47:52.167 | INFO | linux_pty.py: Port endpoint created on /tmp/dev_riot_ble -> /dev/pts/1
16:47:52.167 | INFO | ble_client.py: Receiver set up
16:47:52.803 | INFO | ble_client.py: Trying to connect with DA:ED:02:2A:35:CE: tests/sys/shell/_ble
16:47:54.068 | INFO | ble_client.py: Device DA:ED:02:2A:35:CE connected
16:47:54.068 | INFO | ble_client.py: Found write characteristic ccdd113f-40d5-4d68-86ac-a728dd82f4aa (H. 14)
16:47:54.068 | INFO | ble_client.py: Found notify characteristic 35f28386-3070-4f3b-ba38-27507e991762 (H. 11)
16:47:54.409 | INFO | main.py: Running main loop!

Testing procedure

This is only a documentation change, but the test procedure would be as following:

Make sure you have the latest version of ble-serial installed. Otherwise you can upgrade it with pip install ble-serial --upgrade.
The latest version at the time of writing is 3.0.0.

$ pip show ble-serial
Name: ble-serial
Version: 3.0.0
Summary: Connects BLE adapters with virtual serial ports
Home-page: 
Author: 
Author-email: Jake <[email protected]>
License: MIT License
Location: /usr/local/lib/python3.10/dist-packages
Requires: bleak, coloredlogs
Required-by:

The test in tests/sys/shell_ble runs on all nRF52 devices (and probably others as well with NimBLE support). I used an nRF52840DK and our own hardware.
You can follow the procedure described in the text and should observe the aforementioned error when trying to connect to the device.

@crasbe crasbe requested a review from jia200x as a code owner December 13, 2024 15:45
@github-actions github-actions bot added Area: doc Area: Documentation Area: sys Area: System labels Dec 13, 2024
@crasbe
Copy link
Contributor Author

crasbe commented Dec 13, 2024

Maybe there's a better name for the PR/commit, but I couldn't think of anything better. I'm open to suggestions :D

@benpicco
Copy link
Contributor

huh didn't know this file exist - I think it's contents should be moved to stdio_nimble.h so it shows up in the documentation

@crasbe
Copy link
Contributor Author

crasbe commented Dec 13, 2024

huh didn't know this file exist - I think it's contents should be moved to stdio_nimble.h so it shows up in the documentation

Just as it is, basically Copy & Paste (with some deduplication of what's already there)?

@crasbe
Copy link
Contributor Author

crasbe commented Dec 16, 2024

I don't like necessary line breaks in the code blocks, but otherwise the static tests will bonk me. I did not find a command how to add a line wrap to code blocks in Doxygen.

For example

Line 129-142:
...
 * ```
 * $ ble-serial -d 6BE8174C-A0F8-4479-AFA6-9828372CAFE9 -p /tmp/dev_riot_ble
 * --write-uuid ccdd113f-40d5-4d68-86ac-a728dd82f4aa
 * --read-uuid 35f28386-3070-4f3b-ba38-27507e991762 --write-with-response
 *
 * 17:44:18.765 | INFO | linux_pty.py: Slave created on /tmp/dev_riot_ble -> /dev/ttys006
 * 17:44:18.766 | INFO | ble_interface.py: Receiver set up
 * 17:44:18.766 | INFO | ble_interface.py: Trying to connect with 6BE8174C-A0F8-4479-AFA6-
 * 9828372CAFE9
 * 17:44:19.861 | INFO | ble_interface.py: Device 6BE8174C-A0F8-4479-AFA6-9828372CAFE9 connected
 * 17:44:19.862 | INFO | ble_interface.py: Found write characteristic ccdd113f-40d5-4d68-86ac-
 * a728dd82f4aa (H. 14)
 * 17:44:19.862 | INFO | ble_interface.py: Found notify characteristic 35f28386-3070-4f3b-ba38-
 * 27507e991762 (H. 11)
 * 17:44:19.883 | INFO | main.py: Running main loop!
 * ```
...

We have all this space in the resulting documentation and don't use it:
Bild_2024-12-16_143231407

@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 17, 2024
@riot-ci
Copy link

riot-ci commented Dec 17, 2024

Murdock results

✔️ PASSED

6831f7b fixup! tests/sys/shell_ble: update reference to instructions

Success Failures Total Runtime
1 0 1 01m:18s

Artifacts

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Thanks for spotting and fixing the documentation!

One minor comment below, and tests/sys/shell_ble still references the (now deleted) README. That should be changed to point to the documentation instead.

* $ ble-scan
* Started BLE scan
*
* 6BE8174C-A0F8-4479-AFA6-9828372CAFE9 (RSSI=-40): Riot OS device
Copy link
Contributor

Choose a reason for hiding this comment

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

For me, ble-scan (from ble-serial 3.0.0) shows MAC Adresses instead of UUIDs, e.g. F1:5C:B2:0F:07:1F (rssi=-45): Riot OS device. Not sure if this is only my local environment, but if not, I would change this example here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That depends on the operating system. MacOS X shows the long form, Linux and Windows show MAC addresses.
Perhaps that would be worth mentioning 🤔

I'll change the reference in tests/sys/shell_ble as well.

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants