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 transmit function that returns the mailbox number #25

Merged
merged 4 commits into from
Apr 29, 2021

Conversation

samcrow
Copy link
Contributor

@samcrow samcrow commented Apr 27, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

samcrow commented Apr 28, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

samcrow commented Apr 29, 2021

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
Copy link
Contributor

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
Copy link
Contributor

bors bot commented Apr 29, 2021

Build succeeded:

@bors bors bot merged commit 0b5d782 into stm32-rs:master Apr 29, 2021
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.

3 participants