stm32-rs / bxcan

bxCAN peripheral driver for STM32 chips
Apache License 2.0
31 stars 22 forks source link

Add transmit function that returns the mailbox number #25

Closed samcrow closed 3 years ago

samcrow commented 3 years ago

Motivation

I am developing an implementation of the UAVCAN protocol that uses this bxCAN driver. Each outgoing frame has a transmit deadline. A frame must be discarded if it has not been transmitted by its deadline.

If a frame is placed in a transmit mailbox and later displaced by a higher-priority frame before being transmitted, the software will keep the displaced frame to transmit later. The problem is that there is currently no way to know the deadline of the displaced frame.

Proposed changes

I am proposing to make the smallest possible change that allows this use case to work. This pull request adds a transmit_and_get_mailbox function to the Can and Tx structs. This function is like transmit, but it returns the number of the mailbox that was accessed.

This will allow other code to keep track of the deadline (or other metadata) for the frame in each mailbox, even when frames get displaced.

timokroeger commented 3 years ago

Thank you, looks good to me! Would it make sense to have Tx::abort() as public method to cancel pending frames after the deadline?

samcrow commented 3 years ago

Yes, that would be the only other change needed to make this driver fully compatible with UAVCAN. I've added a public abort function to Tx and Can.

jonas-schievink commented 3 years ago

Thanks, this looks reasonable!

I'm not sure if the transmit_and_get_mailbox API is necessarily the best long-term API to provide, it seems a bit awkward to use a deeply nested return type like that. Perhaps we can fold this into the existing API and have that return something like this instead?

struct TxSuccess {
    mailbox: Mailbox,
    displaced_frame: Option<Frame>,
}

(bikeshedding welcome, I'm not really happy with the name)

Let me know if you'd rather get this merged as-is, this redesign can also be done later if needed.

samcrow commented 3 years ago

That's definitely a more elegant way to do it. I didn't want to propose too many changes for my first pull request here, which is why I suggested the initial tuple version.

Are you suggesting keeping the separate transmit_and_get_mailbox function with TxSuccess as part of its return type, or giving up on transmit_and_get_mailbox and changing the return type of the existing transmit function? I'd be happy to implement either one.

jonas-schievink commented 3 years ago

Are you suggesting keeping the separate transmit_and_get_mailbox function with TxSuccess as part of its return type, or giving up on transmit_and_get_mailbox and changing the return type of the existing transmit function?

I was thinking the latter – change the signature of the normal transmit function to use a new custom struct.

Thinking more about this, I'll go ahead and merge this as-is, since it isn't a breaking change and can be included in 0.5.1. Then we can include the improvements in 0.6.0, since changing transmit's signature is a breaking change either way.

bors r+

bors[bot] commented 3 years ago

Build succeeded: