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 support for Sparkfun XRP #45

Closed
wants to merge 1 commit into from
Closed

Conversation

ThadHouse
Copy link
Contributor

Adds support for https://www.sparkfun.com/products/22230
Copied mostly from the sparkfun-pro-micro

@jannic
Copy link
Member

jannic commented Oct 13, 2023

I just merged #44, migrating the individual Cargo.toml files to use workspace dependencies. Could you change the Cargo.toml you added for the Sparkfun XPR accordingly?

@ThadHouse
Copy link
Contributor Author

@jannic Updated both this and my other 2 PR's as well.

Copy link
Member

@ithinuel ithinuel left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, this is greatly appreciated!

Could you please provide an example demonstrating (some of) the differentiators of this board?

Eg, Something like reading the IMU & printing some defmt traces and controling servos in response.

boards/sparkfun-xrp/src/lib.rs Show resolved Hide resolved
@ThadHouse
Copy link
Contributor Author

Can the examples be done in a later PR? Doing examples for this specifically are kind of invasive and I don't think should block at least the merging of this.

@ThadHouse
Copy link
Contributor Author

The main thing to figure out for the examples is many of them need special functionality. Like the quadrature encoders are a PIO program. That would make sense as a library, but not clear yet if it makes sense to go into the hal, have a new library with each individual thing (This smells way too JS like and I hate it), put it in the BSP, or have it just be a part of the examples.

@ithinuel
Copy link
Member

I initiated a conversation on matrix to gather other people's thoughts.

I don't have strong opinions about the pins for wireless not being used yet because of lack of wifi support.

We are all volunteers with other day jobs. When we accept a crate (a BSP here, or any crate under rp-rs' org), we also (implicitly?) commit to maintaining it and keeping it current with the HAL's and other dependencies' updates.
So the issue I see is that accepting more and more BSPs increases the work load the community has to go through for updates/releases, dutsing it off from time to time.

With this in mind, I'd just like to make sure that when we accept to take care of/"own" a crate, it must at least bring something new/different.
Otherwise people may as-well use an existing crate with the same features but slightly different names.
This is only to "safe-guard" our time and ensure it brings a bit more value to the community.

Others may disagree, and I'm fine to have this merged if they do.

@ithinuel
Copy link
Member

ithinuel commented Oct 15, 2023

[…] Like the quadrature encoders are a PIO program. That would make sense as a library,
but not clear yet if it makes sense to go into the hal, have a new library with each individual […]

This is indeed how PIO features are usually exposed (see i2c-pio, spi-pio, ws2812-pio).

put it in the BSP, or have it just be a part of the examples.

If this is tiny and can fit in 10-20lines, that would be fine IMHO.

If quadrature encoder are too much work, there's still:

I haven't looked at the details of those drivers but they don't need to be feature complete to be used in an example.
If at some point a user wants to update them to showcase a more feature complete driver that's fine too :)

@ThadHouse ThadHouse closed this Oct 17, 2023
@ThadHouse
Copy link
Contributor Author

I think this is actually one BSP my team would probably want to have our own repo for. Theres a lot of functionality on this board and its probably better to have it be standalone.

@jannic
Copy link
Member

jannic commented Oct 17, 2023

We could still add a link to that bsp to the readme if you like.

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