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

Port pygamer to v2 #455

Closed

Conversation

bradleyharden
Copy link
Contributor

@bradleyharden bradleyharden commented Jun 20, 2021

This relies on #467. Please merge it first.

Port the pygamer BSP to the gpio::v2 and sercom::v2::spi APIs. While
implementing this, I discovered that the pygamer uses an undocumented
combination of pins for Sercom1. It uses PA17, PB22 & PB23. The
datasheet indicates that PA17 is in IoSet1, while the other two pins are
in IoSet3. It shouldn't be possible to use these together, but it
appears there is an undocumented IoSet. Add this as UndocIoSet1 for
Sercom1.

Also fix a warning from a trinket_m0 example.

@jacobrosenthal, could you please look this over? In particular, I'd like to draw attention to the various structs within Sets. Since the bsp_pins! macro can provide aliases for pins in a corresponding pin mode, I decided to use those aliases to define the various structs. However, that introduces a change in behavior to the construction of the Sets type. Previously, Sets only distributed pins to the correct structs. But now, creating the Sets struct also changes the pin mode for many pins. Is that ok?

In general, the structure of this BSP might need some thought in light of the v2 changes. For example, the Joystick and JoystickReader structs are now identical, because they both use the pin aliases from bsp_pins!. The Battery and BatteryReader structs are also the same.

Finally, I'd also like to note that I changed the structure of the modules a bit. All of the examples now do this:

use pygamer as bsp;
use bsp::{hal, pac, ...};

I modified the crate to no longer re-export the HAL. I also changing it re-export * from both the buttons and pins modules. I think this is in line with the proposed changes in #357.

@jacobrosenthal
Copy link
Member

@bradleyharden Thanks for the cleanup.

hrm.. it IS meaningfully different..

The previous idea of the sets were that it was naked pins just as if you restarted the board, but grouped for you by functionality and pinout but not configured or costing any more power or startup time than a fresh reset.

Now some stuff can still be init to truly enable it, but in the mean time its all configured which may very well be powering or leaking current into regulators and ics even though youre not using them.

Frankly thought that was true of the old model. Every pin floating could also not be ideal as well.. but at least you knew all the pins were floating..

Im not sure, I guess we have to decide as a repo what we want the bsps to do. To me this basically removes the usefullness of the 'sets' model which I dont know how I feel about (truly, not sure if its good or bad)

@bradleyharden
Copy link
Contributor Author

Let me know what you decide is best. I don't really have an opinion. I can return it to the old approach. It just means more pin aliases.

Speaking of, in terms of the docs, it might make sense for me to create those aliases in a separate module. There can be a lot of them, and they can make it hard to find other items in the docs.

@bradleyharden
Copy link
Contributor Author

One other thing to note. The v1 API had the pins in FloatingInput mode at reset. But that's actually wrong. They are in a floating disabled mode. I don't think it matters for the discussion above, but I wanted to clarify that point.

@jacobrosenthal
Copy link
Member

I rebased this and tested stuff. Most stuff seems all working, but the screen is actually not working even with this ioset5.

Im still not sure on pre configured pins. But maybe the question first needs to be are we keeping the sets model at all or not? It seems like its mainly me using it at this point, and Id rather be consistent than right.

@bradleyharden
Copy link
Contributor Author

@jacobrosenthal, wasn't the undocumented IOSET for the SD card, not the display? I'm not sure what's wrong with the display. Do you have any other symptoms? Maybe you could double check the code and make sure I didn't make any transposition errors?

@bradleyharden bradleyharden force-pushed the port_pygamer_to_v2 branch 2 times, most recently from fdceb21 to 8ee101b Compare June 26, 2021 02:28
@bradleyharden
Copy link
Contributor Author

@jacobrosenthal, I returned the Sets to their original architecture. Could you take another look at the display problem? I have no way to test it.

@bradleyharden bradleyharden force-pushed the port_pygamer_to_v2 branch 3 times, most recently from 1a9bcb3 to 92ecaac Compare June 26, 2021 03:19
@bradleyharden bradleyharden marked this pull request as draft June 26, 2021 03:28
@bradleyharden
Copy link
Contributor Author

I forgot, I wanted to implement the IOSET solution in this PR. Let me do that before we merge.

@bradleyharden bradleyharden marked this pull request as ready for review June 26, 2021 14:44
@bradleyharden
Copy link
Contributor Author

Ok, I think this should be good to go, pending approval by @jacobrosenthal.

@bradleyharden
Copy link
Contributor Author

Whoops, hit the wrong button

@jacobrosenthal
Copy link
Member

Im not sure I reported here, but yes this is still broken. I actually tried the sd card example again as well and thats broken too

Make several improvements to the `sercom::v2::spi` module:
- Introduce a `Registers` type that acts as a task-focused API for the
  registers, as opposed to the register-focused API of the PAC. This
  abstraction also serves to remove interior mutability of the PAC
  struct and lets us implement `Sync` for `Registers`
- Add a `Capability` trait and three corresponding types: `Rx`, `Tx` and
  `Duplex`. Add a `Capability` type parameter to the `Spi` struct and
  use it to differentiate the various embedded HAL trait
  implementations. This is a better solution than the previous approach,
  which was a set of marker traits implemented on `Pads` types.
- Combine the `thumbv6m` and `thumbv7em` modules to reuse more code. The
  major differences between the two chips come in the `Pads` type, and
  the `Length` and `CharSize` types. Introduce a `Size` trait that
  essentially acts a trait alias for either `Length` or `CharSize`.
  Split up the modules and use conditional imports to handle everything
  correctly for the three different chips.

Because the `spi` module is no longer split between the two
chip-specific modules, also consolidate the `impl_pad` modules as well.

Finally, improve the documentation for the `spi` module itself, as well
as for the embedded HAL trait implementations.
Port the pygamer BSP to the `gpio::v2` and `sercom::v2::spi` APIs. While
implementing this, I discovered that the pygamer uses an undocumented
combination of pins for `Sercom1`. It uses PA17, PB22 & PB23. The
datasheet indicates that PA17 is in `IoSet1`, while the other two pins are
in `IoSet3`. It shouldn't be possible to use these together, but it
appears there is an undocumented `IoSet`. Add this as `UndocIoSet1` for
`Sercom1`.

Also fix a warning from a trinket_m0 example.
@jacobrosenthal
Copy link
Member

jacobrosenthal commented Sep 16, 2021

When last we spoke, i was seeing strange reboots on my pygamer, it turns out thats only one device... (the one that has my swd header soldered...) so just not using that one for now and ignoring that problem.

The ferris_img example draw a little garbage on the display then freezes. I cant really scope the screen well because pygamer doesnt break out the tft spi pins.

I bought a thing-plus bsp because it has access to the .1" pins so I could logic analyze it
https://www.sparkfun.com/products/14713

and this very similar (not the exact same) tft screen as pygamer
https://www.amazon.com/gp/product/B07BFV69DZ/ref=ppx_yo_dt_b_search_asin_title?ie=UTF8&psc=1

Needed a patch to hf2 to add the vid/pid
https://github.com/jacobrosenthal/hf2-rs/tree/sparkfun

and ported the bsp and screen example and it works?!
https://github.com/jacobrosenthal/atsamd/tree/port_pygamer_to_v2

technically that chip is the ATSAMD51J20A instead of the ATSAMD51J19A
maybe an eratta? Or the different tft screen is less annoyed with some timing change??

@jacobrosenthal
Copy link
Member

Another note, I stripped the embedded graphics from the sd_card example, and the sdcard spi seems to work fine. So it really is the displays SPI thats giving trouble here I believe (which doesnt use the undoc ioset or anything)

@jbeaurivage
Copy link
Contributor

So does this mean that spi::v2 is actually bug-free?

@bradleyharden
Copy link
Contributor Author

Note that I refactored this to be based on #467, which merged the thumbv6m and thumbv7em versions. That could have fixed a bug in the latter.

@jacobrosenthal
Copy link
Member

It seems to indicate its bug free except when interacting with at least the tft screen on the pygamer

@jacobrosenthal
Copy link
Member

Grabbed a sensepeek so I could probe the FPC on the pygamer. ferris_img here currently just stops drawing writing some noise to like the first 1/8 inch of the screen wipe after leaving the bootloader screen. saleae captures look sane so its not obviously an spi issue (i can see the init sequence https://github.com/sajattack/st7735-lcd-rs/blob/master/src/lib.rs#L88-L90), just have to spot the difference in timing or whatever.
logic.zip

@ianrrees
Copy link
Contributor

Superseded by #750

@ianrrees ianrrees closed this Aug 27, 2024
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.

Bring pygamer up to tier one status
4 participants