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
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion boards/boardsource-blok/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,19 @@ repository = "https://github.com/rp-rs/rp-hal-boards.git"
[dependencies]
cortex-m = "0.7.7"
rp2040-boot2 = { version = "0.3.0", optional = true}
rp2040-hal = { version = "0.8.1"}
rp2040-hal = { version = "0.8.2"}
cortex-m-rt = { version = "0.7.3", optional = true}
fugit = "0.3.5"

[dev-dependencies]
panic-halt = "0.2.0"
embedded-hal = "0.2.7"
nb = "1.0"
heapless = "0.7.16"
smart-leds = "0.3.0"
ws2812-pio = "0.6.0"
usb-device = "0.2.9"
usbd-serial = "0.1.1"
usbd-hid = "0.5.2"
critical-section = "1.1.1"

Expand Down
6 changes: 6 additions & 0 deletions boards/boardsource-blok/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,12 @@ Resets the Blok after 10 seconds to usb boot mode.
Demonstrates emulating a USB Human Input Device (HID) Keyboard. The keyboard
will type "HELLO" five times.

### [blok_usb_serial](./examples/blok_usb_serial.rs)

Demonstrates creating a USB Serial device.
The device will loop 10 times, where on each loop the current loop number is printed.
If the device reads "stop", the blok will reset to usb boot mode.

## Contributing

Contributions are what make the open source community such an amazing place to
Expand Down
160 changes: 160 additions & 0 deletions boards/boardsource-blok/examples/blok_usb_serial.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
//! # USB Serial Example for the Blok
//!
//! Creates a USB Serial device on a blok board, with the USB driver running in
//! the USB interrupt and all writes being done in the main loop with the usage
//! of critical section.
//!
//! This will create a USB Serial device and then write to it.
//! A loop will write its current loop number 10 times after which
//! the Blok will reset to usb boot mode.
//! If "stop" is read by the serial device, the Blok will also
//! reset to usb boot mode.
//!
//! See the `Cargo.toml` file for Copyright and license details.

#![no_std]
#![no_main]

use boardsource_blok::{entry, hal};
use boardsource_blok::{
hal::{
clocks::{init_clocks_and_plls, Clock},
pac,
pac::interrupt,
timer::Timer,
watchdog::Watchdog,
Sio,
},
Pins, XOSC_CRYSTAL_FREQ,
};
use core::fmt::Write;
use heapless::String;
use panic_halt as _;
use usb_device::{
bus::UsbBusAllocator, device::UsbDevice, device::UsbDeviceBuilder, device::UsbVidPid,
};
use usbd_serial::SerialPort;

// shared with the interrupt
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;
Comment on lines +39 to +41
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


#[entry]
fn main() -> ! {
let mut pac = pac::Peripherals::take().unwrap();

let mut watchdog = Watchdog::new(pac.WATCHDOG);

let clocks = init_clocks_and_plls(
XOSC_CRYSTAL_FREQ,
pac.XOSC,
pac.CLOCKS,
pac.PLL_SYS,
pac.PLL_USB,
&mut pac.RESETS,
&mut watchdog,
)
.ok()
.unwrap();

let sio = Sio::new(pac.SIO);
let _pins = Pins::new(
pac.IO_BANK0,
pac.PADS_BANK0,
sio.gpio_bank0,
&mut pac.RESETS,
);

let _timer = Timer::new(pac.TIMER, &mut pac.RESETS);

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

let bus_ref = unsafe { USB_BUS.as_ref().unwrap() };

let serial = SerialPort::new(bus_ref);
unsafe {
USB_SERIAL = Some(serial);
}

let usb_device = UsbDeviceBuilder::new(bus_ref, UsbVidPid(0x1209, 0x0001))
.product("serial port")
.device_class(2) // from: https://www.usb.org/defined-class-codes
.build();
unsafe {
USB_DEVICE = Some(usb_device);
}

unsafe {
pac::NVIC::unmask(hal::pac::Interrupt::USBCTRL_IRQ);
}

let core = pac::CorePeripherals::take().unwrap();
let mut delay = cortex_m::delay::Delay::new(core.SYST, clocks.system_clock.freq().to_Hz());

let mut i: u8 = 0;

loop {
delay.delay_ms(2_000);

let mut text: String<20> = String::new();
writeln!(&mut text, "loop number: {}\r\n", i).unwrap();

write_serial(text.as_bytes());

i += 1;
if i >= 10 {
hal::rom_data::reset_to_usb_boot(0, 0);
}
}
}

/// 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(|_| unsafe {
USB_SERIAL.as_mut().map(|serial| serial.write(byte_array))
})
.unwrap();
}

/// This function is called whenever the USB Hardware generates
/// an Interrupt Request
#[allow(non_snake_case)]
#[interrupt]
unsafe fn USBCTRL_IRQ() {
Comment on lines +133 to +135
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.

let usb_device = USB_DEVICE.as_mut().unwrap();
let serial = USB_SERIAL.as_mut().unwrap();

if usb_device.poll(&mut [serial]) {
let mut buf = [0u8; 64];

match serial.read(&mut buf) {
Err(_e) => {}
Ok(_count) => {
// gets the first 4 bytes of buf
let mut read_text = [0u8; 4];
read_text
.iter_mut()
.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.

Agent59 marked this conversation as resolved.
Show resolved Hide resolved
hal::rom_data::reset_to_usb_boot(0, 0);
} else {
let _ = serial.write("write stop to reset to usb boot\r\n".as_bytes());
}
}
}
}
}