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

Promote the pygamer BSP to Tier 1 status #766

Merged
merged 15 commits into from
Oct 30, 2024

Conversation

kyp44
Copy link
Contributor

@kyp44 kyp44 commented Oct 8, 2024

Summary

Updates PAC references for the pygamer BSP, which was necessary due to #576.

Checklist

  • CHANGELOG.md for the BSP or HAL updated
  • All new or modified code is well documented, especially public items
  • No new warnings or clippy suggestions have been introduced - CI will deny clippy warnings by default! You may #[allow] certain lints where reasonable, but ideally justify those with a short comment.

@jbeaurivage
Copy link
Contributor

Thanks @kyp44! I'll keep this PR on standby for now, I want to make a new pygamer release before merging this to main, since this will mark the BSP's entry into Tier 1 status.

@jbeaurivage
Copy link
Contributor

Closes #513.

@jbeaurivage
Copy link
Contributor

[Follow-up of the conversation here]:

@jbeaurivage I did not update those dependencies, but it would definitely be nice to be able to use the latest versions of those.
EDIT: I don't mind working on updating these (and perhaps any other outdated dependencies) if you like, either in this branch or in a new one. Just let me know.

I think that would be great. We should strive to provide the latest and greatest for T1 boards. I think the embedded-hal v1.0 SPI implementation is a bit buggy at the moment and has some fragile timing and race conditions. I'll work on trying to fix these. Hopefully you can help with testing, since I don't have a pygamer or an LCD display.

@kyp44
Copy link
Contributor Author

kyp44 commented Oct 22, 2024

Here are all the dependencies with major version status:

pygamer library

  • cortex-m: Up to date at 0.7
  • st7735-lcd: 0.8.1 -> 0.10.0
  • cortex-m-rt: Up to date at 0.7
  • usb-device: Up to date at 0.3.1 (latest is 0.3.2, which is just a minor change and so compatible), tied to usb feature

Examples only

  • micromath: 0.5.1 -> 2.1.0
  • embedded-sdmmc: Up to date at 0.8.0
  • embedded-graphics: 0.7.1 -> 0.8.1
  • smart-leds: 0.3 -> 0.4.0
  • lis3dh: 0.1.0 -> 0.4.3
  • tinybmp: 0.3.1 -> 0.6.0

Apparently unused (by the library or examples)

  • cortex-m-rtic: 1.0 -> 1.1.4
  • embedded-hal-02: Specific old version

Dependency questions / issues:

  • micromath is listed as a feature then enabled as a dependency, but this seems to be for no particular reason.
    Nothing in the actual library uses this (it's not even re-exported), and it's only used in a couple of examples. It seems like this should just be moved to the dev dependencies.
  • embedded-sdmmc is the same situation: tied to the sd-card feature, used in an example, but not used in the library at all, not even re-exported.

I imagine that we want to update all of the outdated library and examples dependencies, which I can do and test. I guess the question is whether I should create a separate branch and/or pull request for this, thoughts? Also, what are your thoughts about the issues above for micromath and embedded-sdmmc.

@jbeaurivage
Copy link
Contributor

jbeaurivage commented Oct 23, 2024

Thanks for writeup. I would suggest to work on updating the dependencies here (Could rename the PR to something along the lines of "Promote pygamer to Tier 1 status").

About your questions:

  • micromath and embedded-sdmmc: Can definitely be moved to dev-dependencies
  • cortex-m-rtic seems to be used in the button_rtic example. Most likely it shouldn't be too much effort to update the example to use RTIC v2
  • I think we can yank out embedded-hal v0.2

Some additional thoughts

  • Upgrading st7735-lcd to 0.10 will have the nice addition of using the embedded-hal v1.0 implementations of the HAL.

  • I've recently been reading through our issues, and it seems there are a lot of reports of fragile timing/bad performance for SPI. Particularly, it affects timing-dependent drivers (e.g. neopixels Neopixel examples are problematic #752), and might bring buffer overflows or significant slowdowns at high frequencies.

    I've just opened DMA-enabled embedded-hal blocking implementations #772, which brings DMA transfers for embedded-hal SPI (and later on, I2C and UART) implementations. It would be great if you could test the new DMA impls with the pygamer display, and perhaps the recently removed neopixel examples.

    I don't expect the updated examples or any DMA stuff to make their way into this PR (depends on which will get merged first - probably this one), but it would definitely help build confidence in the implementation, and hopefully enable us to bring the neopixel examples back in a subsequent PR.

@kyp44
Copy link
Contributor Author

kyp44 commented Oct 23, 2024

Copy, I'll work on the updates just in this branch and rename the PR. I'll focus on the core library dependencies first, but it also shouldn't be a big deal to update the example dependencies too, unless this is being worked elsewhere.

I use the neopixels in my project, and they are generally problematic. Evidently they are driven with just a single pin that requires precise timing. The driver allows for various timing options. They actually seemed to work best with the old SpinTimer, which is now fully removed (was previously just deprecated). Currently I'm using a TimerCounter in critical sections because interrupts can screw up the timing for obvious reasons. This works alright but still glitches sometimes with incorrect colors on the LEDs. There is a version of the driver that uses a single SPI pin, but I have tried this, and it sounds like this is also problematic according #752.

Regarding the SPI implementation in general though, I have not experienced any issues or glitches with the display whatsoever. I can look into DMA to test with the display, but the display driver consumes a embedded_hal::spi::SpiDevice directly, so I'm not sure whether that would be compatible with using DMA. That's not so say I couldn't just send some display commands manually using DMA just for testing purposes.

@kyp44 kyp44 changed the title Updates all the PAC references for the pygamer BSP crate Promote the pygamer BSP to Tier 1 status Oct 23, 2024
* Removes some unnecessary features and dependencies.
* Corrects a minor typo in the `pwm_tcc0.rs` example that caused a compilation error.
Dan Whitman and others added 5 commits October 25, 2024 09:58
* micromath: 0.5.1 -> 2.1
* smart-leds: 0.3 -> 0.4
* lis3dh: 0.1.0 -> 0.4.3
* cortex-m-rtic 1.0 -> rtic 2.1.1
* Adds all examples to Cargo.toml per best practice.
* Removes the TODO comments in `Cargo.toml` related to the display/graphics dependency upgrades.
@jbeaurivage jbeaurivage merged commit 250764c into atsamd-rs:master Oct 30, 2024
108 checks passed
@github-actions github-actions bot mentioned this pull request Oct 30, 2024
@kyp44 kyp44 deleted the pygamer_pac branch November 11, 2024 13:45
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