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

cyw43: bluetooth hci support #1820

Closed
wants to merge 27 commits into from

Conversation

brandonros
Copy link
Contributor

@brandonros brandonros commented Aug 24, 2023

i don't know how to finish this up given that:

runner calls bus.init() then takes ownership of it

then

runner has a select3! thing going on that polls ioctl + net_driver_channel + bus.status wait for event interrupt going on, where it runs in its own task

not sure how to attach + expose an HCI connector that would plug into bleps given this

@brandonros brandonros marked this pull request as ready for review September 1, 2023 12:54
@brandonros
Copy link
Contributor Author

@Dirbaio could you please check the comment description of the PR and suggest how i can "plug" this in/give any guidance? to be clear, we're past cyw43 "unknown just need to poke at it" issues and are at the point of extending existing design

@Dirbaio
Copy link
Member

Dirbaio commented Sep 8, 2023

@brandonros haven't looked very deep, but I imagine a design similar to embassy-net-driver-channel could work: allocate packet queues for rx/tx (either a Pipe if it's a byte stream like an uart, or a zerocopy_channel::Channel if it's packet-based.

This allows the cyw43 task to transfer packets between the queues and the cyw43, so a separate BLE task can send/receive packets and handle them with the BLE stack.

Copy link
Contributor

@kbleeke kbleeke left a comment

Choose a reason for hiding this comment

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

Hi! I worked on this driver a bit earlier this year (interrupts among other things). Very excited to see bluetooth support.

I left some comments with my unterstanding of the driver/chip, I hope it helps.

let val = self.read8(FUNC_BUS, REG_BUS_CTRL).await;
trace!("{:#b}", val);

// TODO: C doesn't do this? i doubt it messes anything up
Copy link
Contributor

Choose a reason for hiding this comment

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

This is bascially just a test to ensure that the write to REG_BUS_CTRL sets settings (e.g. endianess) as expected.

.await;

// Enable a selection of interrupts
// TODO: why not all of these F2_F3_FIFO_RD_UNDERFLOW | F2_F3_FIFO_WR_OVERFLOW | COMMAND_ERROR | DATA_ERROR | F2_PACKET_AVAILABLE | F1_OVERFLOW | F1_INTR
Copy link
Contributor

Choose a reason for hiding this comment

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

No point in enabling interrupts that we don't handle.

When I initially implemented this, I just needed IRQ_F2_PACKET_AVAILABLE to stop busy-polling the status register ;)

self.write8(FUNC_BUS, SPI_RESP_DELAY_F1, WHD_BUS_SPI_BACKPLANE_READ_PADD_SIZE)
.await;

// TODO: Make sure error interrupt bits are clear?
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems reasonable, didn't I do this somewhere?


self.state_ch.set_ethernet_address(mac_addr);
if bluetooth_enabled {
// TODO: call runner.init_bluetooth somehow and pass it bluetooth_firmware?
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is here because it has to be done after load_clm?

If there will more bluetooth commands in the future (like turning it off, changing some settings) that are not HCI, you could use a similar construct as the Ioctl module.

This is basicalily how the interaction works. You allocate some space for the response, send an event to the runner. And then wait for the runner to call your waker once the response to the operation is ready.


impl embedded_io_async::Write for HciConnector {
async fn write(&mut self, buf: &[u8]) -> Result<usize, HciConnectorError> {
// TODO: how to get all the way to runner.hci_write()?
Copy link
Contributor

@kbleeke kbleeke Oct 9, 2023

Choose a reason for hiding this comment

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

I don't know anything about HCI but the general idea would probably be to turn the select3 in the runner into select4 with another channel for bluetooth output. So bytes that are to be written here are pushed into a channel. The runner is then awoken on this channel and does the hci_write.

For reads, I assume it starts with an interrupt? After an interrupt, you check why you were interrupted. Previously there was basically only wifi data available. So then you would need another channel/buffer there that you push the data into that you read from a bluetooth event. The other side would then be woken up and can consume the bytes.

trace!("{:#010b}", (val & 0xff));

// 32-bit word length, little endian (which is the default endianess).
// TODO: C library is uint32_t val = WORD_LENGTH_32 | HIGH_SPEED_MODE| ENDIAN_BIG | INTERRUPT_POLARITY_HIGH | WAKE_UP | 0x4 << (8 * SPI_RESPONSE_DELAY) | INTR_WITH_STATUS << (8 * SPI_STATUS_ENABLE);
Copy link
Contributor

Choose a reason for hiding this comment

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

There was probably a reason why I added INTERRUPT_WITH_STATUS even though it might seem unnecessary. I remember, that I would sometimes not get woken up without this setting.

}

async fn bt_bus_request(&mut self) {
// TODO: CYW43_THREAD_ENTER mutex?
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need any mutex here because we cannot get interrupted by the device while doing these operations. (yay cooperative multitasking).

About the interrupt disable, I don't know. If leaving them enabled doesn't corrupt the internal state of the chip, it probably doesn't need to be disabled. We only consume interrupts when we are back on the select3 call.

@isaac-mcfadyen
Copy link

Hey! Any update on this PR? Very excited for BLE support. 😄

@brandonros
Copy link
Contributor Author

probably need somebody else to reattempt this better/nicer/cleaner/get it working

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.

None yet

4 participants