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 some LCD controllers using the FMC/FSMC #297

Merged
merged 3 commits into from
Apr 18, 2021

Conversation

samcrow
Copy link
Contributor

@samcrow samcrow commented Apr 14, 2021

Introduction

This pull request adds code that uses the Flexible Memory Controller or Flexible Static Memory Controller to interface with some types of LCD controllers.

We have another FMC pull request for non-LCD applications that has been inactive for a while. I'm hoping that this pull request will be easier to review because it does not try to implement everything that the hardware supports.

Features

  • Controls up to 4 independent LCDs
  • Compatible with LCD controllers that use the Intel 8080 protocol with a 16-bit bidirectional data bus
  • Includes an example that works on the STM32F412G-DISCO board
  • Provides a low-level interface that can be used to write drivers for specific LCD controllers

Limitations

  • No support for bus widths other than 16 bits
  • I have only tested this code with Sitronix ST7789-series LCD controllers. Are there other LCD controllers that use an Intel 8080-like interface that this code cannot support?
  • The code uses some trait trickery to ensure that the client code can only access the display(s) that correspond to the chip select pin(s) in use. However, this may make the API too difficult for people to understand.

Future work

  • Support an 8-bit data bus (in addition to the currently supported 16-bit bus)
  • Potentially support an 18-bit data bus, on microcontroller models that have more than 16 data pins

@therealprof
Copy link
Member

therealprof commented Apr 14, 2021

Sweet work! I was actually thinking of doing this myself but I've never gotten around to implementing FSMC after the required changes in the SVD files landed. I've posted my FSMC display-interface code here: https://gist.github.com/therealprof/8a374fb08582be613b07f0c371ca3eb9

I would prefer not to add a custom ST7789 driver but either have a completely generic FSMC driver or at least integrate the display-interface driver for more flexibility.

@samcrow
Copy link
Contributor Author

samcrow commented Apr 15, 2021

For clarification, although the st7789-lcd example has some ST7789-specific code, the library itself should work with any LCD controller that supports an Intel 8080-type 16-bit parallel interface. It provides low-level functions that correspond directly to individual transactions on the bus.

I just added some code to make this compatible with the display-interface library.

As far as I can tell, the other common LCD controller interface that the FSMC can support is the Motorola 6800 interface. For details, I looked at the RA8835 LCD controller. It looks like the FSMC can support this interface using mode B with the NADV pin connected to the E input on the LCD controller. The current code supports mode B, but does not support NADV pins.

If the 6800 interface is important, I can add some code to support it. I just don't have the appropriate hardware to test with that interface.

@therealprof
Copy link
Member

I believe some displays can even be switched between different MCU compatibility modes. I do have a ton of different displays, one driver which is also commonly used (also on STM discovery boards) is the ILI9341 for which there's also a display-interface compatible driver 😉.

@therealprof
Copy link
Member

Hm, I'm trying to get your examples to run on a STM32F413 Disco but it hardfaults...

stack backtrace:
   0: HardFaultTrampoline
      <exception entry>
   1: core::ptr::read_volatile
        at /Users/egger/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:1062
   2: stm32f4xx_hal::fsmc_lcd::Lcd<S>::read_data
        at src/fsmc_lcd/mod.rs:336

@therealprof
Copy link
Member

After some more investigation, trying to use any other single CS than 1 will cause a crash.

src/fsmc_lcd/pins.rs Outdated Show resolved Hide resolved
@samcrow
Copy link
Contributor Author

samcrow commented Apr 18, 2021

Thanks for running those tests. I will fix the problems.

@therealprof
Copy link
Member

I'm pretty sure those problems are caused by the lack of initialisation of the other banks but I haven't quite figured out what is missing. Simply initialising btr, bcr and btwr with the same values used for bank1 don't seem to do the trick... I'm kind of glad I already sunk uncounted hours into figuring out how to make it work so I do have a working setup to compare against. 😅

@therealprof
Copy link
Member

I'm pretty sure those problems are caused by the lack of initialisation of the other banks but I haven't quite figured out what is missing. Simply initialising btr, bcr and btwr with the same values used for bank1 don't seem to do the trick... I'm kind of glad I already sunk uncounted hours into figuring out how to make it work so I do have a working setup to compare against. 😅

Haha, bollocks. That was me trying to change to many things at once (i.e. initialising bank3 but trying to use bank4 doesn't work), I can confirm I it works by initialising the correct bank...

@therealprof
Copy link
Member

therealprof commented Apr 18, 2021

I've turned one example of my unreleased display-interface FSMC test code into an standalone example against your code (after manually working around the bank3 the initialisation issues): https://gist.github.com/7e9b6e33b81cc5beb794c12062cf014f

@samcrow
Copy link
Contributor Author

samcrow commented Apr 18, 2021

The code now sets up the configuration and timing registers for "banks" 2, 3, and 4. It looks like those registers really apply to the sub-banks of bank 1, not to the other banks that some microcontroller models have.

With this change, the st7789-lcd example works correctly on my STM32F412G-DISCO board using any of the four chip select signals (specifically PD7 for chip select 1, PG9 for chip select 2, PG10 for chip select 3, or PG12 for chip select 4).

@therealprof
Copy link
Member

The code now sets up the configuration and timing registers for "banks" 2, 3, and 4. It looks like those registers really apply to the sub-banks of bank 1, not to the other banks that some microcontroller models have.

Yeah, the documentation is somewhat convoluted. It seems as if the 4 banks are really independent and tied to the function in use, however if you want to use 4 PSRAM sub-banks you also have to use up all bank configurations even though you only use one bank due to the chip-enabled being tied to the bank configurations.

With this change, the st7789-lcd example works correctly on my STM32F412G-DISCO board using any of the four chip select signals (specifically PD7 for chip select 1, PG9 for chip select 2, PG10 for chip select 3, or PG12 for chip select 4).

I'm still not too hot on the custom included ST7789 driver to be honest. But I think it's probably okay to go ahead with this PR and clean this up further down the road.

Comment on lines 8 to 10
//! We don't know if hardware like that actually exists. Still, this example shows how to compile
//! code that uses multiple address pins and multiple chip select pins.
//!
Copy link
Member

Choose a reason for hiding this comment

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

I'm also no big fan of theoretical examples, I think every example should provide something of immediate value to a potential user.

Comment on lines +196 to +212
// Configure memory type and basic interface settings
// The reference manuals are sometimes unclear on the distinction between banks
// and sub-banks of bank 1. This driver uses addresses in the different sub-banks of
// bank 1. The configuration registers for "bank x" (like FMC_BCRx) actually refer to
// sub-banks, not banks. We need to configure and enable all four of them.
configure_bcr1(&fsmc.bcr1);
configure_bcr(&fsmc.bcr2);
configure_bcr(&fsmc.bcr3);
configure_bcr(&fsmc.bcr4);
configure_btr(&fsmc.btr1, read_timing);
configure_btr(&fsmc.btr2, read_timing);
configure_btr(&fsmc.btr3, read_timing);
configure_btr(&fsmc.btr4, read_timing);
configure_bwtr(&fsmc.bwtr1, write_timing);
configure_bwtr(&fsmc.bwtr2, write_timing);
configure_bwtr(&fsmc.bwtr3, write_timing);
configure_bwtr(&fsmc.bwtr4, write_timing);
Copy link
Member

Choose a reason for hiding this comment

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

That reasoning seems to be not quite correct. As to my interpretation, the configuration of the sub-banks of bank 1 (NOR/PSRAM controller) overlaps with the other banks 2-4 (NAND/PC Card subbanks). That does not mean you need to configure all of them (and indeed we may want to allow other users of FSMC in the future in which case it would cause major conflicts).

Since the common connection between the banks and the sub-banks is the NOE/CS pin we probably can use them as universal owned resource indicating the use of a bank/PSRAM sub-bank and thus tie the configuration of banks to them.

@therealprof
Copy link
Member

Would you like to get this merged as is or squash/rebase to get rid of the sausage making process?

@samcrow
Copy link
Contributor Author

samcrow commented Apr 18, 2021

st7789 driver

When writing the example, I replicated the behavior of the C demonstration code. That code configures several settings on the LCD controller that the st7789 library init function does not. I had to add some code to configure those things. With that done, I almost had a complete driver.

The better approach would probably be to make the st7789 library more flexible.

Multi-LCD example

I moved that example to a documentation comment, which is generally a better place for it. The only problem is that nobody will notice if it becomes out-of-date and no longer compiles.

Enabling banks/sub-banks

Yes, I agree with your understanding about the banks, sub-banks, and pins. We can keep that in mind for later.

I haven't done a squash and rebase before, but I'll try it.

@therealprof
Copy link
Member

st7789 driver

When writing the example, I replicated the behavior of the C demonstration code. That code configures several settings on the LCD controller that the st7789 library init function does not. I had to add some code to configure those things. With that done, I almost had a complete driver.

The better approach would probably be to make the st7789 library more flexible.

I agree that it's not super configurable and 100% complete, but OTOH the driver works very well; I've used it for a half a dozen different displays with that driver IC and it served as a main testing subject for the display-interface crate.

@therealprof
Copy link
Member

I haven't done a squash and rebase before, but I'll try it.

Using git rebase -i does wonders for me. With the fixup command you can easily squash individual commits into fewer.

@samcrow samcrow force-pushed the fsmc-lcd branch 2 times, most recently from 326018d to e7de438 Compare April 18, 2021 21:44
More work on pins, started example

Timing, other things

Example timing, other example functionality

Cleanup, minor fixes

Example cleanup

Minor fixes

Documentation, multiple-LCD example, feature flag

Added fsmc_lcd feature to check.py and CI configuration

Added link to pull request

Fixed column and row ranges for clear function in example

Added support for display-interface, minor documentation improvements

Replaced PC4 with PG12 in documentation

Enabled sub-banks 2, 3, and 4, preventing crashes when using chip select signals 2, 3, and 4

Moved FSMC LCD changelog entry to the correct section
@samcrow
Copy link
Contributor Author

samcrow commented Apr 18, 2021

That was challenging, but I think I have successfully combined everything into three commits without losing any changes.

Copy link
Member

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for sticking with me. I'm very excited about this one! 🎉

bors r+

@bors bors bot merged commit d57eb5f into stm32-rs:master Apr 18, 2021
@samcrow samcrow deleted the fsmc-lcd branch April 22, 2021 05:07
bors bot added a commit that referenced this pull request Jun 1, 2021
312: Fix pin definitions for FSMC LCD interface r=burrbull a=samcrow

I noticed that some of the pin definitions for the FSMC LCD interface that I added in [pull request 297](#297) were wrong. A few of the pins use AF10 when I had assumed that they all use AF12, and pin PD2 should have been included. Here's a fix.

Co-authored-by: Sam Crow <scrow@eng.ucsd.edu>
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