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

Feat: Retry time and count #54

Merged
merged 12 commits into from
Jul 25, 2023

Conversation

elpiel
Copy link
Contributor

@elpiel elpiel commented Jul 12, 2023

Split out from #42

I managed to pull out only this feature from the other PR.
More to follow.

Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
- make crate no_std when not running tests

Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
Copy link
Collaborator

@ryan-summers ryan-summers 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 taking the time to do this! I think we can simplify these changes a lot

src/bus/mod.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/register.rs Outdated Show resolved Hide resolved
///
/// E.g. 4000 = 400ms
#[inline]
pub fn current_retry_timeout(&mut self) -> Result<RetryTime, SpiBus::Error> {
Copy link
Collaborator

@ryan-summers ryan-summers Jul 12, 2023

Choose a reason for hiding this comment

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

What are your thought sabout just making this take a normal int representing millis (or a fugit::Duration)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because then the user of the crate will need to use fugit::Duration and the more complex type can actually be bad when using the crate.

src/uninitialized_device.rs Show resolved Hide resolved
src/uninitialized_device.rs Outdated Show resolved Hide resolved
@elpiel
Copy link
Contributor Author

elpiel commented Jul 12, 2023

@ryan-summers if we don't implement default methods on the Bus trait, then we would have to impl any common register operations (like the RetryCount and RetryTime) twice - DeviceRefMut and UninitializedDevice

Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
@elpiel
Copy link
Contributor Author

elpiel commented Jul 13, 2023

@ryan-summers I'm ok with simplifying the crate if you'd like but abstracting away the registers operations would be better for working on the crate itself.
For me personally it took me some time to get a grip of how those registers should be handled and having docs and custom types helped a lot.

We could, however, hide the RetryCount register and just return/expect u8 in the parameters list for method but for RetryTime I would suggest to leave it as it is. It would even be good to add checks for overflowing the value but I wanted to keep it simple.
Same goes for abstracting the Registers operations ( register to value and value to register) in a trait or smth that all Register structs can use.

Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
@elpiel elpiel force-pushed the feat/retry-time-and-count branch from 9e4ccf1 to 545debf Compare July 13, 2023 09:10
Copy link
Collaborator

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

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

One last question below, otherwise everything looks very good - thanks for taking the time to do the changes!

Comment on lines +118 to +237
)?;

Ok(version_register[0])
}

/// RTR (Retry Time-value Register) [R/W] [0x0019 – 0x001A] [0x07D0]
///
/// # Example
///
/// ```
/// use w5500::register::common::RetryTime;
///
/// let default = RetryTime::from_millis(200);
/// assert_eq!(RetryTime::default(), default);
///
/// // E.g. 4000 (register) = 400ms
/// let four_hundred_ms = RetryTime::from_millis(400);
/// assert_eq!(four_hundred_ms.to_u16(), 4000);
/// ```
#[inline]
pub fn set_retry_timeout(&mut self, retry_time_value: RetryTime) -> Result<(), SpiBus::Error> {
self.bus.write_frame(
register::COMMON,
register::common::RETRY_TIME,
&retry_time_value.to_register(),
)?;

Ok(())
}

/// RTR (Retry Time-value Register) [R/W] [0x0019 – 0x001A] [0x07D0]
///
/// E.g. 4000 = 400ms
#[inline]
pub fn current_retry_timeout(&mut self) -> Result<RetryTime, SpiBus::Error> {
let mut retry_time_register: [u8; 2] = [0, 0];
self.bus.read_frame(
register::COMMON,
register::common::RETRY_TIME,
&mut retry_time_register,
)?;

Ok(RetryTime::from_register(retry_time_register))
}

/// Set a new value for the Retry Count register.
///
/// RCR (Retry Count Register) [R/W] [0x001B] [0x08]
///
/// For more details check out the rest of the datasheet documentation on the Retry count.
///
/// From datasheet:
///
/// RCR configures the number of time of retransmission. When retransmission occurs
/// as many as ‘RCR+1’, Timeout interrupt is issued (Sn_IR[TIMEOUT] = ‘1’).
///
/// The timeout of W5500 can be configurable with RTR and RCR. W5500 has two kind
/// timeout such as Address Resolution Protocol (ARP) and TCP retransmission.
///
/// E.g. In case of errors it will retry for 7 times:
/// `RCR = 0x0007`
pub fn set_retry_count(&mut self, retry_count: u8) -> Result<(), SpiBus::Error> {
self.bus.write_frame(
register::COMMON,
register::common::RETRY_COUNT,
&[retry_count],
)?;

Ok(())
}

/// Get the current Retry Count value
/// RCR (Retry Count Register) [R/W] [0x001B] [0x08]
///
/// E.g. In case of errors it will retry for 7 times:
/// `RCR = 0x0007`
#[inline]
pub fn current_retry_count(&mut self) -> Result<u8, SpiBus::Error> {
let mut retry_count_register: [u8; 1] = [0];
self.bus.read_frame(
register::COMMON,
register::common::RETRY_COUNT,
&mut retry_count_register,
)?;

Ok(retry_count_register[0])
}

#[cfg(not(feature = "no-chip-version-assertion"))]
fn assert_chip_version(
&mut self,
expected_version: u8,
) -> Result<(), InitializeError<SpiBus::Error>> {
let mut version = [0];
self.bus
.read_frame(register::COMMON, register::common::VERSION, &mut version)?;
if version[0] != expected_version {
let version = self.version()?;

if version != expected_version {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do any of these functions need to exist on the UninitializedDevice? Can't we just say "User has to initialize the device before setting retry timers etc."

I don't see a whole lot of value in the code duplication, but I might be missing a usecase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it's per device setting and you might want to set or get the values before you actually initialize the device.

After all, you can device.reset() and get back to the uninitialised state but still want to fetch those values.

Copy link
Collaborator

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

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

Looks fine to me. I don't really think we need the API on UninitializedDevice, but it's also not the worst thing in the world to have.

@ryan-summers ryan-summers merged commit 42791c2 into kellerkindt:master Jul 25, 2023
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