Panic on RP2040 after CC timeout.

[bentwire]

I'm kinda new to rust on embedded, at least with a runtime like RTIC, so I could be completely messed up... Please be gentle :)

Here is the code: https://github.com/bentwire/usb-c-fusb302-pi-rs

Here is the panic:

(HOST) INFO  success!
────────────────────────────────────────────────────────────────────────────────
INFO  i2c0
└─ usb_c_fusb302_pi::app::i2c0_task @ src/main.rs:268
INFO  i2c0
└─ usb_c_fusb302_pi::app::i2c0_task @ src/main.rs:268
INFO  i2c0
└─ usb_c_fusb302_pi::app::i2c0_task @ src/main.rs:268
DEBUG Timeout expired, no CC activity
└─ fusb302b::{impl#1}::poll @ /home/mike/.cargo/git/checkouts/usb-pd-rs-5739c7ec80e1878a/175bf39/fusb302b/src/lib.rs:137
DEBUG Reset
└─ fusb302b::{impl#2}::establish_retry_wait @ /home/mike/.cargo/git/checkouts/usb-pd-rs-5739c7ec80e1878a/175bf39/fusb302b/src/lib.rs:332
INFO  i2c0
└─ usb_c_fusb302_pi::app::i2c0_task @ src/main.rs:268
ERROR panicked at /home/mike/.cargo/git/checkouts/usb-pd-rs-5739c7ec80e1878a/175bf39/fusb302b/src/lib.rs:233:13:
explicit panic
└─ panic_probe::print_defmt::print @ /home/mike/.cargo/registry/src/index.crates.io-6f17d22bba15001f/panic-probe-0.3.1/src/lib.rs:104
────────────────────────────────────────────────────────────────────────────────
(HOST) INFO  program has used at least 1.45/254.47 KiB (0.6%) of stack space
stack backtrace:
   0: HardFaultTrampoline
      <exception entry>
   1: lib::inline::__udf
        at ./asm/inline.rs:181:5
   2: __udf
        at ./asm/lib.rs:51:17
   3: cortex_m::asm::udf
        at /home/mike/.cargo/registry/src/index.crates.io-6f17d22bba15001f/cortex-m-0.7.7/src/asm.rs:43:5
   4: panic_probe::hard_fault
        at /home/mike/.cargo/registry/src/index.crates.io-6f17d22bba15001f/panic-probe-0.3.1/src/lib.rs:86:5
   5: rust_begin_unwind
        at /home/mike/.cargo/registry/src/index.crates.io-6f17d22bba15001f/panic-probe-0.3.1/src/lib.rs:54:9
   6: core::panicking::panic_fmt
        at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/panicking.rs:67:14
   7: core::panicking::panic
        at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/panicking.rs:117:5
   8: <fusb302b::Fusb302b<I2C> as usb_pd::sink::Driver>::poll
   9: usb_pd::sink::Sink<DRIVER>::poll
        at /home/mike/.cargo/git/checkouts/usb-pd-rs-5739c7ec80e1878a/175bf39/usb-pd/src/sink.rs:85:13
  10: usb_c_fusb302_pi::app::i2c0_task::{{closure}}
        at src/main.rs:296:13
  11: rtic::export::lock
        at /home/mike/.cargo/registry/src/index.crates.io-6f17d22bba15001f/cortex-m-rtic-1.1.4/src/export.rs:320:9
  12: usb_c_fusb302_pi::app::<impl rtic_core::Mutex for usb_c_fusb302_pi::app::shared_resources::pdev_that_needs_to_be_locked>::lock
        at src/main.rs:14:1
  13: usb_c_fusb302_pi::app::i2c0_task
        at src/main.rs:292:23
  14: usb_c_fusb302_pi::app::I2C0_IRQ::{{closure}}
        at src/main.rs:14:1
  15: rtic::export::run
        at /home/mike/.cargo/registry/src/index.crates.io-6f17d22bba15001f/cortex-m-rtic-1.1.4/src/export.rs:83:5
  16: I2C0_IRQ
        at src/main.rs:14:1
      <exception entry>
  17: usb_c_fusb302_pi::app::rtic_ext::main::__rtic_init_resources
        at src/main.rs:14:85
  18: cortex_m::asm::nop
        at /home/mike/.cargo/registry/src/index.crates.io-6f17d22bba15001f/cortex-m-0.7.7/src/asm.rs:35:5
  19: main
        at src/main.rs:14:1
  20: Reset
(HOST) ERROR the program panicked```

I'm using a USB C-C cable if that makes a difference.

[fmckeogh]

Interesting, that panic from fusb302b/src/lib.rs:233:13 occurs when the driver is in an unexpected state. I have a refactor to a type state based implementation that would prevent this at compile time, in the mean time I'm not really sure how to help as looking over the code everything seems to be implemented correctly.

Is there any difference if you flash the device with the cable detached, then attach it? I've seen odd behaviour like that before.

[bentwire]

The board isn't wired to allow that unfortunately. Next revision I guess! I did end up adding a "Vendor" command to the list, my laptop advertises DP using a vendor command and it was causing a crash...

I can try out your new impl if you have it somewhere I can get to it. Is it a branch? I did fork your repo to make some small mods to work with latest RP PICO HAL's and RTIC. So playing with patches won't be any issues.

If I can help in any way to get this library working with other MCUs I would love to help! Esp on the RP2040 board I designed for playing with USB PD. :)

[fmckeogh]

@bentwire Oh sorry, I meant the USB-PD device, as in flashing with the USB-C cable detached, then re-attaching?

Just some local changes I'm afraid, but in a totally broken state so nothing worth pushing. The first step is moving timeout and events from the Fusb302b struct into the State enum, this should reduce quite a few bugs as there would be no shared data between different driver states, and each one starts with a known value. The next step would be a pretty standard type state implementation for each variant of State.

Absolutely that sounds awesome, would you like to be added as a maintainer on this repo so you can upstream those changes? It'd definitely be great to make sure this library works across different hardware.

My main focus going forward will be to add source support for the FUSB302B, but there are some more sink features that need to be implemented. I'm also not set on the API, if you have any suggestions for changes to the Driver trait or anything let me know.

[bentwire]

@fmckeogh

I'm still pretty new to Rust, I get the basics but macros and stuff still evade me. If you want to add me as a maintainer that is fine, I would be sending you reviews anyway so I think that's ok?

The biggest change I think that would be helpful is to move the callback in to the library so it isn't necessary. It does not seem very 'rusty' to have a callback like that. I was thinking I could call in to the driver to see what state its in, and to ask it to request a new PDO and to save the list of available PDO's for later query and selection.

Does that make sense?

[fmckeogh]

@bentwire

Sounds good, just ping me and I'll review any PRs.

At the time my rationale was that the callback means we can always respond quickly to power events, we're not reliant on users doing it fast enough to avoid a timeout. The other issue is being able to lookup the PDO index internally, giving a nicer/safer API for users. However, I think you're right and that making poll return an Option<Event>, and then adding a method for requesting power seems like a reasonable alternative?

I'd like to avoid saving the list of PDOs in the library for space reasons, I'd rather let users choose to save them if they wish, but keeping the minimum size of the library down.

[bentwire]

@fmckeogh

That sounds like a perfect compromise to me. Allow for quick response without a callback, and also allow the user to manage the PDO list (Esp since it won't always just be power it seems...)

I can try and work on that if you wish, should have some time this weekend.

Otherwise I will try and put together a diff with just my changes for adding the vendor message and submit a PR.

[fmckeogh]

@bentwire

I should have some time one evening this week, so I'm happy to work on this if you want to work on that PR:)

[fmckeogh]

Any luck with the panic?

[bentwire]

I think so? I modified the code slightly to make sure it was in the right state before calling start sync.

It does not feel quite right to me though:

fn establish_usb_20(&mut self) {
        self.state = State::Measuring { cc_pin: CcPin::CC1 };
        self.events.enqueue(DriverEvent::StateChanged).ok().unwrap();
        //while self.events.dequeue().is_some() {}

        self.start_sink();
    }

[bentwire]

Ok cool, let me get that PR put together.

[bentwire]

Oh here is some happy news:

warning: `usb-c-fusb302-pi` (bin "usb-c-fusb302-pi") generated 7 warnings (run `cargo fix --bin "usb-c-fusb302-pi"` to apply 6 suggestions)
    Finished release [optimized + debuginfo] target(s) in 0.04s
     Running `probe-run --chip RP2040 target/thumbv6m-none-eabi/release/usb-c-fusb302-pi`
(HOST) INFO  flashing program (6 pages / 24.00 KiB)
(HOST) INFO  success!
────────────────────────────────────────────────────────────────────────────────
INFO  INIT!
└─ usb_c_fusb302_pi::app::init::{closure#0} @ src/main.rs:222
INFO  INIT COMPLETE!
└─ usb_c_fusb302_pi::app::init::{closure#0} @ src/main.rs:253
DEBUG CRC ok
└─ fusb302b::{impl#2}::check_for_interrupts @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:291
DEBUG Invalid CRC
└─ fusb302b::{impl#2}::check_for_msg @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:310
DEBUG Invalid CRC
└─ fusb302b::{impl#2}::check_for_msg @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:310
DEBUG Invalid CRC
└─ fusb302b::{impl#2}::check_for_msg @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:310
DEBUG Invalid CRC
└─ fusb302b::{impl#2}::check_for_msg @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:310
DEBUG Invalid CRC
└─ fusb302b::{impl#2}::check_for_msg @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:310
DEBUG Invalid CRC
└─ fusb302b::{impl#2}::check_for_msg @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:310
DEBUG Invalid CRC
└─ fusb302b::{impl#2}::check_for_msg @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:310
DEBUG Good CRC sent
└─ fusb302b::{impl#2}::check_for_interrupts @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:295
DEBUG USB PD comm
└─ fusb302b::{impl#2}::establish_usb_pd @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:409
INFO  CALLBACK
└─ usb_c_fusb302_pi::callback @ src/main.rs:312
INFO  protocol changed
└─ usb_c_fusb302_pi::callback::{closure#0} @ src/main.rs:345
DEBUG CRC ok
└─ fusb302b::{impl#2}::check_for_interrupts @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:291
INFO  CALLBACK
└─ usb_c_fusb302_pi::callback @ src/main.rs:312
INFO  Capabilities changed: 1
└─ usb_c_fusb302_pi::callback @ src/main.rs:315
DEBUG supply @ 0: 5000mV 3000mA
└─ usb_c_fusb302_pi::callback::{closure#0} @ src/main.rs:323
INFO  requesting supply FixedSupply(637636908)@0
└─ usb_c_fusb302_pi::callback::{closure#0} @ src/main.rs:337
DEBUG Good CRC packet
└─ fusb302b::{impl#2}::check_for_msg @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:314
INFO  CALLBACK
└─ usb_c_fusb302_pi::callback @ src/main.rs:312
INFO  power accepted
└─ usb_c_fusb302_pi::callback::{closure#0} @ src/main.rs:346
DEBUG TX ack
└─ fusb302b::{impl#2}::check_for_interrupts @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:280
DEBUG CRC ok
└─ fusb302b::{impl#2}::check_for_interrupts @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:291
DEBUG Good CRC sent
└─ fusb302b::{impl#2}::check_for_interrupts @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:295
DEBUG CRC ok
└─ fusb302b::{impl#2}::check_for_interrupts @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:291
DEBUG Good CRC sent
└─ fusb302b::{impl#2}::check_for_interrupts @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:295
INFO  CALLBACK
└─ usb_c_fusb302_pi::callback @ src/main.rs:312
INFO  power ready
└─ usb_c_fusb302_pi::callback::{closure#0} @ src/main.rs:344
DEBUG VENDOR: 64, 1, [1, 128, 0, 255, 214, 196, 43, 238, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
└─ usb_pd::message::{impl#0}::parse @ /home/mike/devel/rust/embedded/usb-pd-rs/usb-pd/src/message.rs:62
DEBUG Vendor Defined Message! [1, 80, 0, ff, d6, c4, 2b, ee]
└─ usb_pd::sink::{impl#0}::handle_msg @ /home/mike/devel/rust/embedded/usb-pd-rs/usb-pd/src/sink.rs:138
DEBUG CRC ok
└─ fusb302b::{impl#2}::check_for_interrupts @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:291
DEBUG Good CRC sent
└─ fusb302b::{impl#2}::check_for_interrupts @ /home/mike/devel/rust/embedded/usb-pd-rs/fusb302b/src/lib.rs:295

[bentwire]

@fmckeogh Have you been able to find any of the vendor defined message (VDO I think) formats? I want to fix the vendor message so the length is more correct but I can't find any format anywhere...

According to the number of object field in the header there should be 1 object, but it can't be 32bits long because there are obviously at least 8 valid bytes in the packet...

Right now that's the only thing preventing me from putting together a PR with my changes.

[bentwire]

@fmckeogh Finally found something. Created a PR for you to review!

Thanks!

[bentwire]

@fmckeogh Is it OK if I close this for now?

[fmckeogh]

@bentwire Sure, I just pushed a commit replacing the callback API, let me know what you think:)

[bentwire]

@fmckeogh Looks good! Will port to it tonight!

[bentwire]

@fmckeogh Working great!