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 blok_usb_serial example #34

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

Conversation

Agent59
Copy link
Contributor

@Agent59 Agent59 commented Aug 10, 2023

Adds a USB Serial example for the Blok.

Comment on lines +39 to +41
static mut USB_DEVICE: Option<UsbDevice<hal::usb::UsbBus>> = None;
static mut USB_BUS: Option<UsbBusAllocator<hal::usb::UsbBus>> = None;
static mut USB_SERIAL: Option<SerialPort<hal::usb::UsbBus>> = None;
Copy link
Member

Choose a reason for hiding this comment

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

A few unsafe block could be avoided if this were using https://docs.rs/critical-section/latest/critical_section/struct.Mutex.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this good practice? The serial interrupt example of the pico does also use these unsafe blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both UsbDevice and SerialPort reference UsbBus which is why I was not able to store them in multiple Mutexes.
Is this is a readable and viable solution to avoid the unsafe blocks?

// imports

// shared with the interrupt
static STATIC_USB: Mutex<RefCell<Option<StaticUsb>>> = Mutex::new(RefCell::new(None));

struct StaticUsb<'a> {
    usb_bus: UsbBusAllocator<hal::usb::UsbBus>,
    usb_device: Option<UsbDevice<'a, hal::usb::UsbBus>>,
    usb_serial: Option<SerialPort<'a, hal::usb::UsbBus>>,
}

#[entry]
fn main() -> ! {

    // clocks, sio, etc...

    let usb_bus = UsbBusAllocator::new(hal::usb::UsbBus::new(
        pac.USBCTRL_REGS,
        pac.USBCTRL_DPRAM,
        clocks.usb_clock,
        true,
        &mut pac.RESETS,
    ));

    let _ = critical_section::with(|cs| {
        STATIC_USB.replace(cs, Some(StaticUsb { usb_bus, usb_serial: None, usb_device: None }));

        let mut static_usb = STATIC_USB.take(cs).unwrap();
        
        let usb_serial = SerialPort::new(&static_usb.usb_bus);

        let usb_device = UsbDeviceBuilder::new(&static_usb.usb_bus, UsbVidPid(0x1209, 0x0001))
                    .product("serial port")
                    .device_class(2) // from: https://www.usb.org/defined-class-codes
                    .build();

        static_usb.usb_serial = Some(usb_serial);
        static_usb.usb_device = Some(usb_device);
    });

    // main loop
}

/// Writes to the serial port.
///
/// We do this with interrupts disabled, to avoid a race hazard with the USB IRQ.
fn write_serial(byte_array: &[u8]) {
    let _ = critical_section::with(|cs| {
        STATIC_USB.take(cs).map(|usb| {
                usb.usb_serial.map(|mut serial| serial.write(byte_array))
            })
    });
}

// interrupt

Copy link
Member

Choose a reason for hiding this comment

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

take removes the value from its container (see Mutex::take pointing to RefCell::take.
You should use Mutex::borrow_ref_mut instead.
But yes, that's the idea.

Copy link
Member

Choose a reason for hiding this comment

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

I'll be creating a PR this evening (BST) to update the rp-pico's usb_serial_interrupt example to show a more comprehensive and up-to-date example.

Copy link
Member

Choose a reason for hiding this comment

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

See: #37

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much

.enumerate()
.for_each(|(i, e)| *e = buf[i]);

if &read_text == b"stop" {
Copy link
Member

Choose a reason for hiding this comment

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

if bytes are received 1 by 1, this will not work as expected.

This behaviour is dependant on many factors such as:

  • Host platform (windows/linux/macos etc)
  • serial port tool (Hercules, putty, minicom, screen etc)

Also, although I think the reading of the serial port needs to be done in the interrupt in order to garanty no data loss may occur, the processing of that data would be better off in the main loop rather than the interrupt.
In this specific case, it does not really mater, but it's always good to demonstrate good practices in examples not to confuse less seasoned developers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid the issue with bytes being received 1 by 1 should they be stored between interrupts or is there a more optimal way?

There is an interrupt serial example for the pico and the processing of the data was also kept in the interrupt handler, should this then be changed as well?

Copy link
Member

Choose a reason for hiding this comment

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

You would either manage a state machine that steps forward when the next character of stop is received but that might be a bit too heavy for the purpose of that example :D

A static buffer should be enough. Whether the static buffer is owned locally to the interrupt or global, I don't really mind.

Comment on lines +133 to +135
#[allow(non_snake_case)]
#[interrupt]
unsafe fn USBCTRL_IRQ() {
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, this is not sufficient to remain compliant with USB's spec.
There also need to be a time based interrupt to garanty the IRQ is polled at least every 10ms.

See: https://docs.rs/usb-device/latest/usb_device/device/struct.UsbDevice.html#method.poll

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It says there that it should be called "preferably from an interrupt handler.". And I'm not sure where you read that it's not sufficient to remain compliant with USB's spec and what you mean by that. I'm quite new to programming, please help me out.

Copy link
Member

Choose a reason for hiding this comment

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

It should be called preferably from an interrupt handler in order to minimise the latency between the event and its processing.
The next sentence is:

Must be called at least once every 10 milliseconds while connected to the USB host to be USB compliant.

The USB interrupt is not guaranteed to be called at any interval (eg IIRC during usb-suspend, there's no USB interrupt).
The usb-device's stack may need to update its state machine eg to handle timeouts, disconnection or some internal resource allocation.
It may even not currently (or no longer) strictly need it, but still keep this requirement to future-proof the crate 'just in case'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for all the questions and thank you for your time.

If I understand correctly the usb interrupt shouldn't be used to poll the usb-device because it has to keep getting updated even if the host doesn't request any data.
But shouldn't the pico serial interrupt example then be changed as well?
Would this mean that usb interrupt shouldn't be used at all and the usb-device has to be called as often as possible in the main loop?
How would you then allow e.g. sleeping in the main loop?

I'm once again sorry, but i don't understand what "it" and "this" refer to in your last sentence.

Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly the usb interrupt shouldn't be used to poll the usb-device because it has to keep getting updated even if the host doesn't request any data.

The usb-interrupt should be used to poll the usb-device to guaranty minimal latency but it should also be polled on a time based schedule. It could be from an interrupt or from the main loop.

But shouldn't the pico serial interrupt example then be changed as well?

It's well possible. Some of the examples predate the critical-section support etc.

How would you then allow e.g. sleeping in the main loop?

If you want to sleep in the main loop but still wake at set interval, you can use the cortex_m::asm::wfe or cortex_m::asm::wfi and configure your system accordingly to generate an event (via timer, alarm or systick) an wake the core.

I'm once again sorry, but i don't understand what "it" and "this" refer to in your last sentence.

My apologies, let me rephrase that sentence:

The usb-device stack may even not currently (or no longer) strictly need this time-based polling, but still keep this requirement (the time-based polling) to future-proof the usb-device crate 'just in case'.

I hope this all help.

boards/boardsource-blok/examples/blok_usb_serial.rs Outdated Show resolved Hide resolved
@ithinuel
Copy link
Member

Thank you for your contribution.

I left a few comments & suggestions.

@Agent59 Agent59 marked this pull request as draft August 15, 2023 18:12
@Agent59
Copy link
Contributor Author

Agent59 commented Aug 15, 2023

Does it still make sense to add this serial example for the blok, considering that your (ithinuel's) pull request #37 would give the exact same example for the pico, which should also work on the blok?

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.

2 participants