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

Implement transceiver delay compensation configuration #57

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

Conversation

liamkinne
Copy link
Contributor

Tested on an STM32G474, working up to 10 Mbit/s. Without this, the controller won't ack any frames above 3 Mbit/s.

Copy link
Member

@richardeoin richardeoin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Cool that it's working up to 10 Mbit/s

src/lib.rs Show resolved Hide resolved
@liamkinne
Copy link
Contributor Author

@richardeoin Do you want it to be a bit-and like tdc & 0x7F or an assert to enforce the range?

@richardeoin
Copy link
Member

I'd recommend using saturating_mul for the multiplication, followed by core::cmp::min to limit it to 128.

@liamkinne
Copy link
Contributor Author

@richardeoin looking at the STCubeG4 implementation it looks like the TDCO value is written directly to the register with no need for offsetting by -1.

I assume this is because zero is a valid value unlike dtseg1, dtseg2, etc where it is not.

@richardeoin
Copy link
Member

The smallest possible result from btr.dtseg1() * btr.dbrp() is 1. As zero is a valid value, a minus one is needed to ever write 0 to the TDCO field.

@liamkinne
Copy link
Contributor Author

I can see where you're coming from. I'm just not able to find any evidence in other implementations that this is the case.

This example is where I got the calculation from.

The implementation just writes the value straight to the register without any offsetting of the value.

Sorry to be a pain about this. I just want to make sure it's correct because it's not arbitrarily configurable.

@richardeoin
Copy link
Member

I can also merge it like this. In practice +1 or -1 on the value of TDCO won't make a difference to the receiver.

Btw you merged the latest master into the branch of this PR. Typically you would rebase this branch onto master, instead of the merge. That way the changes from the other PR aren't included here. Not a major issue though :)

@liamkinne liamkinne force-pushed the transceiver-delay-compensation branch from e95bf77 to e95f8c4 Compare February 3, 2025 01:43
@liamkinne liamkinne force-pushed the transceiver-delay-compensation branch from e95f8c4 to d5d9dd0 Compare February 3, 2025 01:45
@liamkinne
Copy link
Contributor Author

@richardeoin thanks. I've fixed up the commit history.

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.

2 participants