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 a dummy usb bus implementation #152

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

sourcebox
Copy link
Contributor

This PR adds DummyUsbBuswhich doesn't implement any functionality but can be used when writing examples (e.g. on class implementations) to ensure they compile. Note that any code using it cannot be run though.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Hmm, I understand the utility but this crate has a whole bunch of traits and I am slightly worried about potentially adding a lot of dummy implementations.
Do we have many examples where this code would need to be duplicated?

@sourcebox
Copy link
Contributor Author

Without a dummy implementation, a real HAL is needed to compile the code. So you can't just setup an example in your class crate that is checked for syntax correctness. A good example is the current usbd-serial crate that has an example in the readme that is outdated and there's no way to have it checked. So a user that copies it from there will end up with errors thrown.

A working version of that example using the dummy implementation could be like this:

use usb_device::{
    bus::UsbBusAllocator,
    device::{UsbDeviceBuilder, UsbVidPid},
    dummy_bus::DummyUsbBus,
    prelude::*,
};
use usbd_serial::{SerialPort, USB_CLASS_CDC};

fn main() {
    let usb_bus = UsbBusAllocator::new(DummyUsbBus::new());

    let mut serial = SerialPort::new(&usb_bus);

    let mut control_buffer = [0; 256];

    let string_descriptors = [StringDescriptors::new(LangID::EN_US).product("Serial port")];

    let mut usb_dev =
        UsbDeviceBuilder::new(&usb_bus, UsbVidPid(0x16c0, 0x27dd), &mut control_buffer)
            .strings(&string_descriptors)
            .unwrap()
            .device_class(USB_CLASS_CDC)
            .build()
            .unwrap();

    loop {
        if !usb_dev.poll(&mut [&mut serial]) {
            continue;
        }

        let mut buf = [0u8; 64];

        match serial.read(&mut buf[..]) {
            Ok(count) => {
                // count bytes were read to &buf[..count]
            }
            Err(UsbError::WouldBlock) => todo!(), // No data received
            Err(err) => todo!(),                  // An error occurred
        };

        match serial.write(&[0x3a, 0x29]) {
            Ok(count) => {
                // count bytes were written
            }
            Err(UsbError::WouldBlock) => todo!(), // No data could be written (buffers full)
            Err(err) => todo!(),                  // An error occurred
        };
    }
}

@eldruin
Copy link
Member

eldruin commented Jun 12, 2024

Yes, I understood the motivation. My question is if it is worth providing dummy implementations inside this crate, instead of adding it in-place if we have a very limited number of examples (for the documentation, this dummy implementation could be added hidden so that it is actually compiled but not shown, like we do over at embedded-hal)
Also, I wonder if we will want to add dummy implementations for the rest of traits provided here.

use usb_device::{
    bus::UsbBusAllocator,
    device::{UsbDeviceBuilder, UsbVidPid},
    dummy_bus::DummyUsbBus,
    prelude::*,
};
use usbd_serial::{SerialPort, USB_CLASS_CDC};

fn main() {
    let usb_bus = UsbBusAllocator::new(DummyUsbBus::new());

    let mut serial = SerialPort::new(&usb_bus);

    let mut control_buffer = [0; 256];

    let string_descriptors = [StringDescriptors::new(LangID::EN_US).product("Serial port")];

    let mut usb_dev =
        UsbDeviceBuilder::new(&usb_bus, UsbVidPid(0x16c0, 0x27dd), &mut control_buffer)
            .strings(&string_descriptors)
            .unwrap()
            .device_class(USB_CLASS_CDC)
            .build()
            .unwrap();

    loop {
        if !usb_dev.poll(&mut [&mut serial]) {
            continue;
        }

        let mut buf = [0u8; 64];

        match serial.read(&mut buf[..]) {
            Ok(count) => {
                // count bytes were read to &buf[..count]
            }
            Err(UsbError::WouldBlock) => todo!(), // No data received
            Err(err) => todo!(),                  // An error occurred
        };

        match serial.write(&[0x3a, 0x29]) {
            Ok(count) => {
                // count bytes were written
            }
            Err(UsbError::WouldBlock) => todo!(), // No data could be written (buffers full)
            Err(err) => todo!(),                  // An error occurred
        };
    }
}

// In the docs we can prepend all this with # to hide it
struct DummyUsbBus;

impl DummyUsbBus {
    fn new() -> Self {
        Self
    }
}

impl UsbBus for DummyUsbBus {
    fn alloc_ep(
        &mut self,
        ep_dir: crate::UsbDirection,
        ep_addr: Option<crate::class_prelude::EndpointAddress>,
        ep_type: crate::class_prelude::EndpointType,
        max_packet_size: u16,
        interval: u8,
    ) -> crate::Result<crate::class_prelude::EndpointAddress> {  }
    fn enable(&mut self) {   }
    fn force_reset(&self) -> crate::Result<()> {  }
    fn is_stalled(&self, ep_addr: crate::class_prelude::EndpointAddress) -> bool {  }
    fn poll(&self) -> crate::bus::PollResult {   }
    fn read(
        &self,
        ep_addr: crate::class_prelude::EndpointAddress,
        buf: &mut [u8],
    ) -> crate::Result<usize> {  }
    fn reset(&self) {   }
    fn resume(&self) {   }
    fn set_device_address(&self, addr: u8) {  }
}

@sourcebox
Copy link
Contributor Author

I understand that this dummy implementation feels a bit strange, but I have no better idea how to make testing easy for class crate authors. Rust in general motivates people to write examples that actually build, so this is just a convinient way to support this. I see it a bit similar to the test class that already exists in the crate.

The alternatives are:

  • Do it locally inside the class crate (as you suggested). That has the disadvantage of the need to get into something that class authors normally don't do: dealing with the bus. Also any changes of the UsbBus trait have to be tracked. Overall, there's code duplication/maintainance in the class crates.
  • Doing a separate dummy bus crate. That would add another dependency to a specific version of usb-device, something that is already problematic.

@eldruin
Copy link
Member

eldruin commented Jun 12, 2024

Hmm, I understand the situation better now, thanks.
This is similar for driver crates, where I add a dev dependency on linux-embedded-hal so that I can get an I2C/SPI bus in the examples and docs. For that price, I get examples that compile on the popular Raspberry Pis. If there is not such a little-hassle HAL impl and platform for usb-device, I understand how the situation is more complicated.

Doing away with this by providing a dummy implementation first-party is certainly alluring from an engineering perspective. Conversely, the examples will compile but they need to be translated to a real HAL impl, which is a non-trivial burden.

If my recollection of the situation is correct, I am inclined to accept this (although maybe rename the module just dummy for possible future extension).
What do you think @ryan-summers ?

@ryan-summers
Copy link
Member

ryan-summers commented Jun 13, 2024

My only request is that we gate the dummy implementation behind a feature to prevent people from using it in actual projects (or at least hide it).

In the past, I've maintained these as separate crates (i.e. std-embedded-nal), but it has been a painful maintenance process. I like keeping it in a single location.

@sourcebox
Copy link
Contributor Author

sourcebox commented Jun 13, 2024

My only request is that we gate the dummy implementation behind a feature to prevent people from using it in actual projects (or at least hide it).

A good idea in general, but I'm not sure how the class crate authors should use it i.e. whether

  • use a single dependency with the feature enabled?
  • use a separate dev-dependency and enable the feature only there?

@sourcebox
Copy link
Contributor Author

One problem with features in general is though that patch.crates-io doesn't support them irc. Unfortunately, this crate is somewhat prone to be force-patched to ensure that all involved crates use the same version.

@vitalyvb
Copy link
Contributor

vitalyvb commented Jul 1, 2024

Having a dummy bus would have been useful for one documentation example I had to write, but it's not a huge trait, and basically IDE does it automatically anyway (link). If would be significantly more useful if there were a lot of examples needing the bus, though.

src/dummy_bus.rs Outdated Show resolved Hide resolved
@ryan-summers ryan-summers merged commit 87c9626 into rust-embedded-community:master Jul 2, 2024
3 checks passed
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