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

embassy_stm32/eth: support compliance testing #3355

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

Gerharddc
Copy link

@Gerharddc Gerharddc commented Sep 19, 2024

This change adds the possibility to perform Ethernet compliance testing with STM32 systems when using a custom PHY struct to generate test patterns.

@Gerharddc
Copy link
Author

It seems I've only modified eth v1, I still need to modify eth v2 but it would be great to already get some feedback on the PR if possible.

@Gerharddc
Copy link
Author

On second thought, it is probably better to add a separate struct called EthernetTester (or similar) which consumes the Ethernet struct in order to enter test mode. It would then have a function to give back the Ethernet struct which would reset the PHY first. This would then leverage the type system to ensure you don't hand over Ethernet to the networking stack while the PHY is still in test mode.

@Dirbaio
Copy link
Member

Dirbaio commented Sep 19, 2024

I'd prefer to avoid extending the PHY trait with methods for every single feature one might want from a PHY.

The trait exists so you can customize the behavior of SMI operations the ethernet driver needs to do. For example, the embassy-net-driver trait requires Eth to be able to report whether the link is up or down. This means Eth has to know how to get this info out of the phy, and since it varies based on the phy it's a trait method so that it can be customized by the user.

This is not the case for compliance test mode. The Eth driver never needs to call it, only the end user. So there's no need for including these functions in the trait.

I think exposing access to SMI instead should be enough for the user to do any SMI operation they want, without having to extend the trait for each possible feature:

impl Eth {
    fn station_management(&mut self) -> &mut impl StationManagement {
        &mut self.station_management
    }
}

This change adds the possibility to perform compliance testing with
STM32 systems by directly exposing SMI when needed. Users can
then use this to configure PHY registers for test modes.
@Gerharddc
Copy link
Author

@Dirbaio thanks for the feedback! I agree that it is indeed much cleaner to simply expose SMI directly and have made the suggested change. I opted to mark the function as unsafe though since I believe it could unexpectedly mess things up and most users probably should not be accessing the SMI directly under normal circumstances.

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

thanks!

@Dirbaio Dirbaio added this pull request to the merge queue Sep 20, 2024
Merged via the queue into embassy-rs:main with commit 3020bf3 Sep 20, 2024
6 checks passed
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