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 Nordic Uart Service (NUS) peripheral to examples #141

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

Conversation

plaes
Copy link
Contributor

@plaes plaes commented Oct 17, 2024

Add generic peripheral implementation of Nordic Uart Service (NUS) and add example of it to nrf-sdc.

Has still some TODOs

@plaes plaes force-pushed the nrf-uart-example branch 2 times, most recently from 585208e to 0d1c873 Compare October 23, 2024 10:51
let conn = advertiser.accept().await?;

/* TODO: Implement "echo" and push rx bytes back to tx? */
// XXX: I have to manually "fix" the size from 123 -> 128
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lulf Any clues what I'm doing wrong here? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@plaes Can you explain a little more about the issue? Does something break if you make the tx array 123 bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you have following on line 118, it asserts that 123 != 128 when sending the notification after someone has connected:

let mut tx = [0; ATT_MTU];

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting...I'm guessing this is the asset in question:

assert_eq!(value.len(), input.len());

The issue you're facing is because your tx characteristic is of type heapless::Vec<u8, 123>, while the local variable tx is of type [u8; 123]. heapless::Vec has 4 bytes of metadata associated with it, giving 127 bytes which is probably being padded to 128 for alignment :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Following up on this, I'd definitely recommend using the same type throughout. With your code as it is currently written, line 124 is actually modifying the first byte in the heapless::Vec's length field (which is of type usize). This will cause undefined behaviour if you were to try to use it as a Vec elsewhere.

On the whole I would be reluctant to use non-primitive types for characteristics as it assumes that the other end of the comms channel has an understanding of that type. Furthermore, the heapless implementation of Vec could change its memory structure in a future version, which would change the representation of the data in memory and cause further confusion.

TL;DR: in this instance I'd strongly recommend making your tx characteristic a byte array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where the assertion happens:

pub fn set(&self, handle: Characteristic, input: &[u8]) -> Result<(), Error> {
self.iterate(|mut it| {
while let Some(att) = it.next() {
if att.handle == handle.handle {
if let AttributeData::Data { props, value } = &mut att.data {
assert_eq!(value.len(), input.len());
value.copy_from_slice(input);
return Ok(());

Regarding nrf-softdevice, all the value conversions seem to go through to_gatt() and from_gatt() methods, which is provided by two traits: FixedGattValue and GattValue.

GattValue can handle data with length smaller than attribute size and also implements support for Vec and String types via Heapless.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, I suspect we'll want to just implement the same, or possibly do so as part of the bt-hci crate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I'm in the process of bringing the FixedGattValue and GattValue traits into TrouBLE as part of my work on #136. The purpose of that work isn't to implement support for variable length characteristics, but it will provide the framework to be able to implement this in future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've raised #149 to track the variable length characteristic implementation

Copy link
Member

Choose a reason for hiding this comment

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

Agreed bringing over FixedGattValue and GattValue traits to trouble makes sense. There is also #133 which might also allow doing varlength without having to store it.

@plaes
Copy link
Contributor Author

plaes commented Oct 23, 2024

Updated example to the new API, but I have to dig into the whole NRF UART thingy a bit more to understand how to properly test it. And secondly, there's this buffer size modification that I need to perform - not sure I'm doing this properly or this should be handled via some kind of wrapper method that handles size requirements.

@jamessizeland
Copy link
Collaborator

what hardware do I need to try this out?

@plaes
Copy link
Contributor Author

plaes commented Oct 23, 2024

what hardware do I need to try this out?

I'm currently on custom nrf52832 board, but it should be possible to use the generic "apps" part on any hardware you have as UART isn't actually connected to any peripheral.

@jamessizeland
Copy link
Collaborator

are you blocked by anything on finishing this PR that we can help with @plaes?

@plaes
Copy link
Contributor Author

plaes commented Nov 27, 2024

are you blocked by anything on finishing this PR that we can help with @plaes?

Not really blocked, but busy with other things. If you want, you can take over.

@jamessizeland
Copy link
Collaborator

Cool, no worries, I'll see about taking a look!

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.

4 participants