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.

Sign up for GitHub

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

Defmt log / Unknown cmd #7

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Defmt log / Unknown cmd #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d7a3c2
Allow disable defmt logging while still deriving Format
alexkazik Sep 22, 2024
449f201
Report the unknown command
alexkazik Sep 22, 2024
35ebb2f
fixup, if ok: wrapper to show command in hex
alexkazik Sep 22, 2024
bf3a101
Fix typos in docs
alexkazik Sep 23, 2024
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
13 changes: 7 additions & 6 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ Currently, only `Bulk Only` transport is implemented. It is possible to implemen
# Features
This crate has a couple of opt-in features that all could be used independently.

| Feature | Description |
|---------|------------------------------------------------------------------|
| `bbb` | Include Bulk Only Transport |
| `scsi` | Include SCSI subclass |
| `ufi` | Include USB Floppy Interface sublcass |
| `defmt` | Enable logging via [defmt](https://crates.io/crates/defmt) crate |
| Feature | Description |
|-------------|------------------------------------------------|
| `bbb` | Include Bulk Only Transport |
| `scsi` | Include SCSI subclass |
| `ufi` | Include USB Floppy Interface subclass |
| `defmt` | Derive [defmt](https://crates.io/crates/defmt) |
| `defmt-log` | Log internal messages via defmt |

# Examples
See [examples](examples)
1 change: 1 addition & 0 deletions usbd-storage/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ default-features = false
[features]
default = []
defmt = ["dep:defmt", "usb-device/defmt"]
defmt-log = ["defmt"]
bbb = []
ufi = []
scsi = []
Expand Down
12 changes: 6 additions & 6 deletions usbd-storage/src/fmt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
macro_rules! trace {
($s:literal $(, $x:expr)* $(,)?) => {
{
#[cfg(feature = "defmt")]
#[cfg(feature = "defmt-log")]
Copy link
Owner

Choose a reason for hiding this comment

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

The idea was to rely on defmt's filtering mechanism: https://defmt.ferrous-systems.com/filtering#disabling-logs
So one would enable a defmt feature on a crate and then compile a binary with DEFMT_LOG=trace,usbd-storage=off cargo run --bin app
Do you have a different use-case?

Copy link
Author

Choose a reason for hiding this comment

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

I'm very new to defmt, if that is the usual way, I'll use it no problem

::defmt::trace!($s $(, $x)*);
#[cfg(not(feature="defmt"))]
#[cfg(not(feature="defmt-log"))]
let _ = ($( & $x ),*);
}
};
Expand All @@ -15,9 +15,9 @@ macro_rules! trace {
macro_rules! info {
($s:literal $(, $x:expr)* $(,)?) => {
{
#[cfg(feature = "defmt")]
#[cfg(feature = "defmt-log")]
::defmt::info!($s $(, $x)*);
#[cfg(not(feature="defmt"))]
#[cfg(not(feature="defmt-log"))]
let _ = ($( & $x ),*);
}
};
Expand All @@ -26,9 +26,9 @@ macro_rules! info {
macro_rules! debug {
($s:literal $(, $x:expr)* $(,)?) => {
{
#[cfg(feature = "defmt")]
#[cfg(feature = "defmt-log")]
::defmt::debug!($s $(, $x)*);
#[cfg(not(feature="defmt"))]
#[cfg(not(feature="defmt-log"))]
let _ = ($( & $x ),*);
}
};
Expand Down
5 changes: 3 additions & 2 deletions usbd-storage/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@
//! | ------- |---------------------------------------|
//! | `bbb` | Include Bulk Only Transport |
//! | `scsi` | Include SCSI subclass |
//! | `ufi` | Include USB Floppy Interface sublcass |
//! | `defmt` | Enable logging via [defmt](https://crates.io/crates/defmt) crate |
//! | `ufi` | Include USB Floppy Interface subclass |
//! | `defmt` | Derive [defmt](https://crates.io/crates/defmt) |
//! | `defmt-log` | Log internal messages via defmt |
//!
//! [usb-device]: https://crates.io/crates/usb-device
//! [SCSI]: crate::subclass::scsi
Expand Down
25 changes: 23 additions & 2 deletions usbd-storage/src/subclass/scsi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use crate::transport::Transport;
use crate::CLASS_MASS_STORAGE;
use core::fmt::{Debug, Formatter};
use num_enum::TryFromPrimitive;
use usb_device::bus::InterfaceNumber;
use usb_device::bus::UsbBus;
Expand Down Expand Up @@ -40,14 +41,34 @@ const WRITE_10: u8 = 0x2A;
/* MMC */
const READ_FORMAT_CAPACITIES: u8 = 0x23;

/// A u8, when printed shown as hex
#[repr(transparent)]
#[derive(Copy, Clone)]
pub struct U8Hex(pub u8);
Copy link
Owner

Choose a reason for hiding this comment

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

I like the idea of printing in hex, however I would expand it to all ScsiCommands and UfiCommands and therefore for u16, u64 and u32.
The newtype is an obvious choice but I wonder if we could rely on https://defmt.ferrous-systems.com/hints#propagation instead. If I understand it correctly, then by adding something like :#X to src/fmt.rs macros, we get Hex formatting everywhere for free?

I'm not sure that derive(defmt::Format) would add =u8-like typing info, so maybe it would require an explicit implementation of defmt::Format for ScsiCommand and UfiCommand? (may put them in fmt.rs)

As a last resort, I would still try to implement newtypes for other primitives and see how much of a code change it requires. (I don't mind changing a public API. that's what semver is for :) )

Copy link
Author

Choose a reason for hiding this comment

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

I don't think propagation does help, as entries with display hint (= show hex) can't be overwritten. Also only a struct (or so) can implement Format, but since the command is a u8 it can't be formatted without newtype.

Yes, I should add =u8, and probably use {=u8:#04x} instead of 0x{:02x}.

Copy link
Owner

Choose a reason for hiding this comment

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

I tried

#[derive(defmt::Format)]
enum FakeCommand {
    Unknown {
        u16: u16,
        u32: u32,
        u64: u64,
        u128: u128,
    }
}

#[cortex_m_rt::entry]
fn main() -> ! {
    let foo = FakeCommand::Unknown{
        u16: 0xCAFE,
        u32: 0xCAFE,
        u64: 0xCAFE,
        u128: 0xCAFE,
    };
    defmt::info!("{:#X}", foo);
}

and it printed
Unknown { u16: 0xCAFE, u32: 0xCAFE, u64: 0xCAFE, u128: 0xCAFE }.

So if :#X hints are added to fmt.rs macros, then crate-internal logging will output everything in hex.

Note, that this does not currently work with u64 and u128. Hopefully knurling-rs/defmt#869 will be merged


impl Debug for U8Hex {
fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
write!(f, "0x{:02x}", self.0)
}
}

#[cfg(feature = "defmt")]
impl defmt::Format for U8Hex {
fn format(&self, fmt: defmt::Formatter) {
defmt::write!(fmt, "0x{:02x}", self.0)
}
}

/// SCSI command
///
/// Refer to specifications (SPC,SAM,SBC,MMC,etc.)
#[derive(Copy, Clone, Debug)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
#[non_exhaustive]
pub enum ScsiCommand {
Unknown,
Unknown {
cmd: U8Hex,
},

/* SPC */
Inquiry {
Expand Down Expand Up @@ -151,7 +172,7 @@ fn parse_cb(cb: &[u8]) -> ScsiCommand {
READ_FORMAT_CAPACITIES => ScsiCommand::ReadFormatCapacities {
alloc_len: u16::from_be_bytes([cb[7], cb[8]]),
},
_ => ScsiCommand::Unknown,
cmd => ScsiCommand::Unknown { cmd: U8Hex(cmd) },
}
}

Expand Down