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

Ethernet merge #121

Merged
merged 7 commits into from
Aug 14, 2020
Merged

Ethernet merge #121

merged 7 commits into from
Aug 14, 2020

Conversation

hargoniX
Copy link
Member

@hargoniX hargoniX commented Aug 9, 2020

This PR adds stm32h7-ethernet into the HAL behind the ethernet feature flag, trying to introduce as little diff as possible in the process of doing so.

Closes: #102

memory.x Show resolved Hide resolved
@ryan-summers
Copy link
Member

@hargoniX / @richardeoin After getting this in, I would like to hear your potential thoughts on the following:

  1. Should we pull the phy support out (or provide a generic trait such that users can provide their own phy support if required)?
  2. Let's clean up the pins definition - we should be able to take a Pins structure in the constructor similar to other peripherals
  3. I'd really like to clean up the unsafe-ness here, but I'm not familiar as to why its originally there.

Curious to hear your thoughts here - we can break this out into an issue if necessary.

@richardeoin
Copy link
Member

  1. I agree it would be nice to pull out the PHY support. The PHY trait is there already in ethernet/mod.rs, but currently we use the feature flags to choose an implementation, rather than allowing the user to pass generic object that implements PHY. Definitely fixable.

  2. Good point, I'd missed that we don't offer a way to check the pins. Definitely we should keep the same new /new_unchecked pattern.

  3. I think there's always going to be some unsafe because of the DMA interaction. But we can definitely reduce it, especially in the public API. A good place to start would be ethernet_init, I don't think it actually needs to be unsafe. But I think users would need unsafe to take the DesRing from static storage?

Since we plan rework the PHY code, calling this 'shared' doesn't make sense to
me
Matches other peripherals in the HAL
@richardeoin
Copy link
Member

@ryan-summers Do you want to merge this now, and then continue with those 3 points in another PR?

@ryan-summers
Copy link
Member

ryan-summers commented Aug 12, 2020 via email

@richardeoin
Copy link
Member

Ok!

bors r+

@bors bors bot merged commit 1179d0c into stm32-rs:master Aug 14, 2020
@richardeoin richardeoin mentioned this pull request Aug 31, 2020
4 tasks
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.

Reincorporate stm32h7-ethernet
3 participants