df8oe / UHSDR

SDR firmware and bootloader with configuration files for use with Eclipse, EmBitz and Makefile
Other
358 stars 189 forks source link

Discussion: 1.Display module mess, 2. Added RTOS. #1625

Closed m-chichikalov closed 5 years ago

m-chichikalov commented 5 years ago

I would like to discuss couple topics, not sure that this is the best place for it, if not let me know where is it... Bear with me guys, please.

"Displays mess"

Do we really need to check (recognize) the display on the run time? We would end up that even 2mb is not enough. Maybe better to compile several different binaries for each supported display? Or I do not understand something? As long I want to add support for another display, I see that there is such big mess in a display module. Seems like some steps were done to be abstract but not 100%

"All done in INT context."

Another point what I do not like that everything, almost everything, is done in interrupts, so it's a trap guys... There is no way to easily maintenance a system like this. What do you thinking about some RTOS? I'm sure .... someone already did it and got that there is no enogh power resources of CMU or memmory? I do not have mcHF HW but I saw videos and number of MCU usage on the screen was around 34% this is true?

I know, when in Rome, do as the Romans do, however... Does someone have thoughts about it? Share please, let's discuss

PS. Maybe do you have another place for topics like this?

sp9bsl commented 5 years ago

Hi Max, I will shortly answer you with the "display mess" because I'm partially responsible for this. The unified driver we have today has one advantage: one executable for all machines with the same core, regardless of used display. In our opinion this way has an advantage becuse creates less mess for the users. Of course the code might be worse or better written, we continously work at it, you are welcome if want to help. Currently there is unified driver for 320x240 and 480x320 and almost ready for 800x480. The last one will be available only for F7/H7 cores.

I highly recommend you to have mcHF or better OVI40 UI hardware before introducing any changes to code. The best place to discuss new/improved ideas is our forum. Please register there and we can discuss, this tab is mainly for resolving the issues.

73 Slawek

m-chichikalov commented 5 years ago

I highly recommend you to have mcHF or better OVI40 UI hardware before introducing any changes to code.

Sure, that's why I did not touch anything yet )))

The best place to discuss new/improved ideas is our forum. Please register there and we can discuss, this tab is mainly for resolving the issues.

I will, thanks for the link.

73 Slawek

73!

db4ple commented 5 years ago

Hi, a couple of things: 1) Display Driver: If you look closely you can see that we can disable each driver and hence code should go away, similarily we handle the layout info. It is not perfect but for the time being it is a sufficient solution as the drivers do not contribute much code anyway.

2) Doing a lot in an interrupt. General textbook wisdom is not to run much code in the interrupt. This is to achieve low latency for interrupt handling. The approach selected by the original developers was to have a big fat main loop, some not too critical interrupts and the audio interrupt which does the real-time audio processing. That is a valid choice with a lot of drawbacks when it comes to more complex scenarios and UI latency (which is handled in the main loop). WIth a few optimizations and changes it is able to handle the whole application now reasonable well. If you look closely, we in fact have kind of simple priority based scheduling implemented: HW Interrupts, high priority tasks but not interrupts we run in the PendSV Handled and all the rest in the main loop. One main benefit of this approach is the simplicity in terms of memory allocation: We have only a single stack. No need to worry about multiple stacks for the different tasks/threads. Running most UI code in the main loop also limits the number of critical sections we have to synchronize. Drawbacks are also obvious, it is difficult to have code running concurrently to achieve lower latency for some actions. But so far it works somewhat ok.

And yes, a not so low overhead RTOS would bring us to the limits on the 192k STM32F4 at least if we have FreeDV in which is a memory eater. Some very lightweight like FreeRTOS would not be a problem in that respect but would require some thinking nevertheless.

You may want to watch https://gaming.youtube.com/watch?t=789&v=CLC4VWjHdfw were we present a little bit of the philosophy behind the UHSDR firmware development approach..

But as Slawek said, for further discussion, please use the forum.

m-chichikalov commented 5 years ago

It was pleasure to watch some of you guys... Thank you for video... For somereason I cannot register in the forum, it does not like my email probably...

@db4ple, I noticed... you mention about RTOS atleast twice in the video)))

at 6th of December going to porticipate workshop of ST relaited to GFX, maybe will pick up some ideas for display module or something other... 73!

m-chichikalov commented 5 years ago
  1. Display Driver: If you look closely you can see that we can disable each driver and hence code should go away, similarily we handle the layout info. It is not perfect but for the time being it is a sufficient solution as the drivers do not contribute much code anyway.

I've just undefined USE_GFX_ILI932x and build crashed. Not a big deal...

Also would be nice to comunicate with guys via Skype or something like this... my skype is rv9yw_max Welcome

db4ple commented 5 years ago

Indeed, you hit a bug in our defines. Is already fixed.

df8oe commented 5 years ago

@m-chichikalov : You are already registered in the forum, but you are right: outlook.com refuses to receive emails from me (and all other servers hosted at Hetzner here in Germany). It is a very bad behaviour of absolutely incompetent antispam technic. Also used by hotmail, aol, gmail, yahoo :) In the future email addresses at these providers only can communicate against each other ;)

m-chichikalov commented 5 years ago

@df8oe Hi, Andreas, could I ask you to remove my account... I will create another one with different email. Thank you.

df8oe commented 5 years ago

done...

m-chichikalov commented 5 years ago

@db4ple Hi, take a look please... are they should be like this ->

#ifdef USE_GFX_ILI932x
        {
                DISPLAY_HY28B_PARALLEL, "HY28A/B Para.",
                .ReadDisplayId = UiLcdHy28_ReadDisplayId_ILI932x,
                .SetActiveWindow = UiLcdHy28_SetActiveWindow_ILI932x,
                .SetCursorA = UiLcdHy28_SetCursorA_ILI932x,
                .WriteRAM_Prepare = UiLcdHy28_WriteRAM_Prepare_ILI932x,
        },
#ifdef USE_GFX_SSD1289
        {
                DISPLAY_HY32D_PARALLEL_SSD1289, "HY32D Para. SSD1289",
                .ReadDisplayId = UiLcdHy28_ReadDisplayId_SSD1289,
                .SetActiveWindow = UiLcdHy28_SetActiveWindow_SSD1289,
                .SetCursorA = UiLcdHy28_SetCursorA_SSD1289,
                .WriteRAM_Prepare = UiLcdHy28_WriteRAM_Prepare_ILI932x,
        },
#endif // USE_GFX_SSD1289
#endif // USE_GFX_ILI932x

USE_GFX_SSD1289 inside of USE_GFX_ILI932x they are working together or why is it like this? Looks like you are not able to use SSD1289 when ILI932x disabled.

m-chichikalov commented 5 years ago

One more question for today: There is a define USE_SPI_DMA. Does FW has display driver wich is using SPI but not with DMA?

m-chichikalov commented 5 years ago

One more:

// we support HY28A SPI only on the UI_BRD_MCHF
#if defined(USE_GFX_ILI932x) && defined(USE_SPI_DISPLAY) && defined(UI_BRD_MCHF)
        {
                DISPLAY_HY28A_SPI, "HY28A SPI",
                .ReadDisplayId = UiLcdHy28_ReadDisplayId_ILI932x,
                .SetActiveWindow = UiLcdHy28_SetActiveWindow_ILI932x,
                .SetCursorA = UiLcdHy28_SetCursorA_ILI932x,
                .WriteRAM_Prepare = UiLcdHy28_WriteRAM_Prepare_ILI932x,
                .WriteDataSpiStart_Prepare = UiLcdHy28_WriteDataSpiStart_Prepare_ILI932x,
                .WriteIndexSpi_Prepare = UiLcdHy28_WriteIndexSpi_Prepare_ILI932x,
                LCD_D11_PIO, LCD_D11, true, false
        },
#endif // defined(USE_GFX_ILI932x) && defined(USE_SPI_DISPLAY) && defined(UI_BRD_MCHF)
#if defined(USE_GFX_ILI932x) && defined(USE_SPI_DISPLAY)
        {
                DISPLAY_HY28B_SPI, "HY28B SPI",
                .ReadDisplayId = UiLcdHy28_ReadDisplayId_ILI932x,
                .SetActiveWindow = UiLcdHy28_SetActiveWindow_ILI932x,
                .SetCursorA = UiLcdHy28_SetCursorA_ILI932x,
                .WriteRAM_Prepare = UiLcdHy28_WriteRAM_Prepare_ILI932x,
                .WriteDataSpiStart_Prepare = UiLcdHy28_WriteDataSpiStart_Prepare_ILI932x,
                .WriteIndexSpi_Prepare = UiLcdHy28_WriteIndexSpi_Prepare_ILI932x,
                .spi_cs_port = LCD_CSA_PIO,
                .spi_cs_pin = LCD_CSA,
                true, false
        },
#endif // defined(USE_GFX_ILI932x) && defined(USE_SPI_DISPLAY)

Two absolutely identical displays except Name strings and pins for chip select of SPI. So, maybe remove one of them and just change pins definition depends on UI_BRD_MCHF?

db4ple commented 5 years ago

Hi, HY28A SPI and HY28B SPI have different pinouts on the display module and both need to be supported on the mcHF. The pin routing needed for HY28A is only available on the mcHF hence this driver is restricted to the mcHF board. The pin routing for the HY28B is identical to all other SPI displays / display adapters hence we can support this on all platforms. No reason for change.

SSD1289 inside ILI932x -> Well, the SSD1289 driver is not (yet) working anyway, it is work in progress one could say (if this one is more of the optimisti nature), and it uses a function of the ILI932x. So don't take this too serious. It was my doing and I can't really remember why I did this, but for sure I haven't been working on this since it seems I am the only one with a HY28D with a SSD1289 controller. I now also have HY28D with the "right" controller which somewhat stopped me from completing this work it seems. The right choice would be to have the SSD1289 commented out of the official builds for now and if it works, separate the two.

USE_SPI_DMA is no enabled on the STM32H7. It is not so much a question of the driver not using SPI per driver as an porting issue, see https://github.com/df8oe/UHSDR/blob/active-devel/mchf-eclipse/drivers/ui/lcd/ui_lcd_hy28.c#L28 .

When porting and/or debugging some issues, the ability to turn off DMA has proven to help.

The fact that on the H7 platform SPI_DMA is turned off is due to the very limited interest of using SPI displays if you can use the parallel one without sacrifice (on the mcHF you have to run the display in SPI if you want the RTC due to the just 100 pin MCU). So no one verified the DMA configuration to work and it remains turned off until someone tests this. To be honest, when I ported the firmware to the H7, I tested it without DMA and then did not care to test it with DMA since I don't think anyone will use it either way.

db4ple commented 5 years ago

BTW, well spotted!

m-chichikalov commented 5 years ago

Thank you. It came a bit clear... I've started reconstruct all this display concrete module... Would be nice if DF8OE create 'test_displays_banchmark' for me, I would like to do PR into that branch and latter we will ask comunity to test all variants...

I have thoughts ( actually not thoughts, it's already implemented in my another project ) how to make SPI displays work much faster...

About this code

// we support HY28A SPI only on the UI_BRD_MCHF
#if defined(USE_GFX_ILI932x) && defined(USE_SPI_DISPLAY) && defined(UI_BRD_MCHF)
        {
                DISPLAY_HY28A_SPI, "HY28A SPI",
                .ReadDisplayId = UiLcdHy28_ReadDisplayId_ILI932x,
                .SetActiveWindow = UiLcdHy28_SetActiveWindow_ILI932x,
                .SetCursorA = UiLcdHy28_SetCursorA_ILI932x,
                .WriteRAM_Prepare = UiLcdHy28_WriteRAM_Prepare_ILI932x,
                .WriteDataSpiStart_Prepare = UiLcdHy28_WriteDataSpiStart_Prepare_ILI932x,
                .WriteIndexSpi_Prepare = UiLcdHy28_WriteIndexSpi_Prepare_ILI932x,
                LCD_D11_PIO, LCD_D11, true, false
        },
#endif // defined(USE_GFX_ILI932x) && defined(USE_SPI_DISPLAY) && defined(UI_BRD_MCHF)
#if defined(USE_GFX_ILI932x) && defined(USE_SPI_DISPLAY)
        {
                DISPLAY_HY28B_SPI, "HY28B SPI",
                .ReadDisplayId = UiLcdHy28_ReadDisplayId_ILI932x,
                .SetActiveWindow = UiLcdHy28_SetActiveWindow_ILI932x,
                .SetCursorA = UiLcdHy28_SetCursorA_ILI932x,
                .WriteRAM_Prepare = UiLcdHy28_WriteRAM_Prepare_ILI932x,
                .WriteDataSpiStart_Prepare = UiLcdHy28_WriteDataSpiStart_Prepare_ILI932x,
                .WriteIndexSpi_Prepare = UiLcdHy28_WriteIndexSpi_Prepare_ILI932x,
                .spi_cs_port = LCD_CSA_PIO,
                .spi_cs_pin = LCD_CSA,
                true, false
        },
#endif // defined(USE_GFX_ILI932x) && defined(USE_SPI_DISPLAY)

change it to this and we are fine...

// we support HY28A SPI only on the UI_BRD_MCHF
#if defined(USE_GFX_ILI932x) && defined(USE_SPI_DISPLAY)
        {
                DISPLAY_HY28B_SPI, "HY28B SPI",
                .ReadDisplayId = UiLcdHy28_ReadDisplayId_ILI932x,
                .SetActiveWindow = UiLcdHy28_SetActiveWindow_ILI932x,
                .SetCursorA = UiLcdHy28_SetCursorA_ILI932x,
                .WriteRAM_Prepare = UiLcdHy28_WriteRAM_Prepare_ILI932x,
                .WriteDataSpiStart_Prepare = UiLcdHy28_WriteDataSpiStart_Prepare_ILI932x,
                .WriteIndexSpi_Prepare = UiLcdHy28_WriteIndexSpi_Prepare_ILI932x,
#if defined(UI_BRD_MCHF)
                .spi_cs_port = LCD_D11_PIO,
                .spi_cs_pin = LCD_D11,
#else 
                .spi_cs_port = LCD_CSA_PIO,
                .spi_cs_pin = LCD_CSA,
#endif
                true, false
        },
#endif // defined(USE_GFX_ILI932x) && defined(USE_SPI_DISPLAY)

No?

BTW, I've purchased OVI40 UI kit with display.

db4ple commented 5 years ago

Hi, you're proposed change is unfortunately not right. On the mcHF UI BRD and only on the mcHF we need BOTH definitions to be present. HY28A SPI and HY28B SPI both work on the mcHF, each is slightly differently wired hence two different definitions.

HY28A wiring is not supported on OVI40, thus we don't need the HY28A entry for the OVI40 board

m-chichikalov commented 5 years ago

Ok, I see your point but take a look : When UI_BRD_MCHF is defined only DISPLAY_HY28A_SPI included in build. https://github.com/df8oe/UHSDR/blob/6a7a12148cae69c967521ce71c31736a3c44514c/mchf-eclipse/drivers/ui/lcd/ui_lcd_hy28.c#L2166

m-chichikalov commented 5 years ago

You need remove lines 2177 and 2178 + 2191 to have it like you said. Am I right?

db4ple commented 5 years ago

Sorry, now I get what you mean. But the solution is a little different. See #1629

m-chichikalov commented 5 years ago

@db4ple Hi, could I ask you For you it's much easy to answer, then me to track it down.

Do some functions like UiLcdHy28_PrintTextCentered, UiLcdHy28_LcdClear and so on, are called from interrupt context? I hope not...

db4ple commented 5 years ago

UiLcd... functions and in fact all Ui... functions MUST NOT be called in an interrupt. Never ever. They are not designed for that.

The only place to call functions drawing on the screen is the main loop.