search

danielgallagher0 / bluetooth-hci

Bluetooth HCI implementation for embedded Rust
Apache License 2.0
53 stars 8 forks source link

HciHeader packet type byte #1

Open achan1989 opened 5 years ago

achan1989 commented 5 years ago

Hi, nice crate :)

I read your doc comments about the HciHeader trait and the one-byte packet type that precedes it. I think I can offer some insight into what's going on, and some kind of "ideal" solution (although it may be too much for what you're trying to do with the crate).

Problem

Fundamentally, the "top" of HCI allows four kinds of packet to be transferred -- command, ACL data, sync data, event (vol 2, part E, sec 5.4). A command packet header is just three bytes, and that's it. Crucially, the top of HCI is supposed to be transport-agnostic. The packet type byte that you have included in the header is a transport-level concern (specific to a particular kind of UART transport), and you've got this issue because you're mixing layers and responsibilities.

As a counter-example, look at the USB transport (vol 4, part B, sec 2.1.1). Table 2.1 shows that, for USB, the transport separates the four kinds of HCI packet into different interfaces and endpoints. There is no need for a packet type byte, because the kind of packet can be determined at the transport layer without any additional information.

Possible Solution

You'd have to change the Controller trait. Instead of just one write function you'd need write_command, write_acl, and write_sync. It's up to the device crate to define how they work. The read direction would probably need something similar.

The uart::CommandHeader and cmd_link::Header structs can then be collapsed down into one struct with three bytes.

Then a device crate that implements a Controller with a UART transport would add the appropriate packet type byte whenever write_x was called, before writing the data out of the UART.

A device crate that implements a Controller with a USB transport would just write the data out to the appropriate interface/endpoint.

Misc

This would be a significant and breaking change to your crate, but it's a cleaner API and abstraction in the end. I was interested in using this crate as the basis for a USB HCI driver -- I think it's still possible in its current state, but the API would make things a bit messy.

I haven't thought much about the read direction, but it seems more difficult. I don't yet understand the reason for splitting HCI into host::Hci and uart::Hci, or the reason why there's cmd_link, event_link and uart.

danielgallagher0 commented 5 years ago

Hi, thanks for your insights!

As I mentioned in the comments, I hadn't encountered any hosts that had a non-UART interface, so my design is necessarily speculative. I can explain what I was aiming at, and you can let me know if you think it would work that way. Also, I don't mind having breaking changes. I versioned this 0.0.x because I knew it would need to change. :-)

(It's been a while since I've written this, so it's all a bit fuzzy. I'm trying to figure out what I was thinking...)

I think the cmd_link and event_link were an attempt to send commands and read events, respectively, without the packet type byte. Now that I look back at it, I don't know that they will work with a single type using or implementing both.

host::Hci defines the bulk of the implementation, and is written in terms of a generic Header type. uart, cmd_link, and event_link were intended to provide implementations for Header: uart adds the packet type byte, and the others don't. So the transport implementation can use uart::CommandHeader instead of handling the packet type internally.

If you think splitting up write into multiple functions, I'd be happy to give that a try. I think I would still provide a UART implementation (though perhaps in a new crate).

Do you have a device in mind that I could play around with and test the reworked implementation? Or do you want to try updating the code and submitting PRs? I'm open to either. I only have a handful of hours per week I can work on this, but I would be happy to help get this working for you.

achan1989 commented 5 years ago

I don't have a UART/embedded device that I can run this on, but eventually I should be able to test any changes on a USB/Linux device (once I've written the appropriate device crate). So nothing useful yet, sorry.

It sounds like you've actually used this crate on an embedded device. How did you schedule the receive actions? I.e. how do you know when to call Controller::read_into()? I'm an embedded software guy in my day job if that helps, although my experience with embedded Rust is very limited.

I ask because I'm tempted to try hacking on this and see what I can come up with, but I don't want to do something that is incompatible with an embedded environment. I think I'll need to play a bit to figure out what does and doesn't work.

danielgallagher0 commented 5 years ago

I only have one device crate (bluenrg), and I have a simple test application that reads on a fixed timer. I should be able to connect the BlueNRG's data ready signal to an external interrupt, but I haven't bothered yet. I'll try to do that when I write a non-toy application.