Add blok_usb_serial example

[Agent59]

Adds a USB Serial example for the Blok.

[ithinuel]

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

[Agent59]

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

[Agent59]

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

[ithinuel]

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.

[ithinuel]

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.

[ithinuel]

See: #37

[Agent59]

Thank you very much

[ithinuel]

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.

[Agent59]

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?

[ithinuel]

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.

[ithinuel]

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

[Agent59]

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.

[ithinuel]

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'.

[Agent59]

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.

[ithinuel]

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.

[ithinuel]

Thank you for your contribution.

I left a few comments & suggestions.

[Agent59]

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?