Splitting Graphics Driver Code

[db4ple]

I would ask the developers for commenting on a largish refactoring change. I would like to separate the UI driver low level driver code from the code which handles the drawing API. That means we will have one file with basically these functions (for handling the controller level operations)

void    UiLcdHy28_BulkPixel_OpenWrite(ushort x, ushort width, ushort y, ushort height);
void    UiLcdHy28_BulkPixel_CloseWrite();
void    UiLcdHy28_BulkPixel_Put(uint16_t pixel);
void    UiLcdHy28_BulkPixel_PutBuffer(uint16_t* pixel_buffer, uint32_t len);
void    UiLcdHy28_BulkPixel_BufferFlush();

uint8_t 	UiLcdHy28_Init();

and another one with all the Print & Draw functions.

That would involved significant renaming at least in a second stage. In the first round I would just split these files.

I would propose to keep the controller side of things in the original file, and move the rest to a new file set called "ui_draw.c/.h"

Any objections? Suggestions?

Rationale behind this is a growing number of controllers we support and the actually clear separation between both parts, so it is safe and reduces problems when making changes in one or the other part.
There is no performance impact to be expected.

[db4ple]

A side note: I would think we keep the touchscreen code in the controller side of things for the moment, these are fairly related and the touch screen stuff is not too large.

[m-chichikalov]

Hi Danilo, this is for me? Take a look my repo if you interested how I do it so far. The major side and idea which I keep in my mind when do it, to broke as much as I can dependencies and make it easy to adopt RTOS later. Right now the graphical part ready for using in concurrency environment. I've changed that transaction start only when first buffer is ready, so there is no delay to prepare buffer.... and others changes, for sure I will prepare some documentation, dont worry.

[m-chichikalov]

A side note: I would think we keep the touchscreen code in the controller side of things for the moment, these are fairly related and the touch screen stuff is not too large.

I do not agree with you. It should be separate module and there is nothing related to display's controllers.

[m-chichikalov]

And BTW, for SPI I achieved improvements around 8-15% for different operations

12500 --> 11705
2991 --> 2148
3332 --> 2531
1016 --> 697
4571 --> 3622
4572 --> 3622
7112 --> 6033
8667 (not correct) --> 3018

and I think to implement Init functions to try both prescalers to be sure that the person this good display using it on higher clock.

I implemented mem-to-mem DMA for F4. Performance increased a bit. going to optimize all print text functions, for now they looks not optimized.

[sp9bsl]

Danilo, agree. Function names are borrowed from first/old approach of Chris and it looks a bit strange when function name begins with old display name (HY28_...) and we use it for ILI9486 or RA8875. I was thinking about this move a month or two ago...

Maksim, keeping the touchscreen with display controller is a key part, so I would not agree with you. The reason is simple: RA8875 has controller for resistive panels inside the display chip and uses the same IO as the display controller, the RPi lcd (doesn't matter how frequently used) also shares the same SPI port.

[m-chichikalov]

That they use common interface is not reason to keep them together. Guys let's start thinking about dependencies, concurrency and stop writing the spaghetti code.

How I managed SPI for LCD and Touch:

First of all I added simple locks mechanism, which can be easely replaced later if we go to RTOS.
locks.c

/**
 * Semaphore give
 */
void sem_give( volatile bool* sem )
{
	*sem = false;
//	xSemaphoreGiveFromISR( *sem, pdFALSE );
}

/**
 * Semaphore take, may suspend the caller thread
 */
int sem_take( volatile bool* sem, uint timeout )
{
	(void)( timeout );
	// As long as we do not have any concurrency,
	// there is no way that semaphore is already taken by someone.
	// The below implementation for DMA or interrupt driven Functions,
	// they should give back semaphore after it's done.
	if ( !*sem )
	{
		*sem = true;
		return ERR_NONE;
	}
	while ( *sem )
	{
		//FIXME maybe add timeout for more robust code?
		asm("nop");
	}
	*sem = true; // take semaphore after it's released...
	return ERR_NONE;
//	return xSemaphoreTake( *sem, timeout ) ? ERR_NONE : ERR_TIMEOUT;
}

Further I added SPI interface - like boundary to separate HW depended code from aplication.
it has the next interface:

const IO_t SPI_IO = {
	.RequestTransaction = UHSDR_SPI_RequestTransaction,
	.CloseTransaction   = UHSDR_SPI_CloseTransaction,
	.Send_DMA           = UHSDR_SPI_Send_DMA,
	.Send_Polling       = UHSDR_SPI_Send_Polling,
	.Receive_Polling    = UHSDR_SPI_Receive_Polling,
	.Receive_DMA        = UHSDR_SPI_Receive_DMA,
};

const IO_t* UHSDR_SPI_GetIO( void )
{
	return (const IO_t*)&SPI_IO;
}

the same for FMC and display driver are using one of them depends on interface it's using.

Go back to SPI display and SPI touch:

Any transaction to the Display should start from taking its semaphore. For now it's just bool type field in Display struct.
By this we are protecting our buffers from corruption when Interrupt or DMA approach are in invoked.

Each interface also has semaphore, so before start transaction over SPI the
IO->RequestTransaction ( uint Timeout, void* ExtraParameter );
shoud be called.
if used polling function of interface the
IO->CloseTransaction();
should be called manually
if DMA was used the close transaction would be called automatically from IRQ.

the code you can look at my repo... it's not done yet but more or less...

I also add into SPI interface implementation of
IO->RequestTransaction ( uint Timeout, void* ExtraParameter );
that it takes the struct of used chip select pin, so it set it back automatically when transaction over DMA is done. The main code do not need to handle CS anymore. And does not matter who is calling it touch or display or third party which using the same SPI...

The same done for FMC. Your 8875 display and touch inside it just should concurently share the same interface and respect agrement to take semaphore and give it back.

so, it has not only touch controler inside but also others, for ex. buttons controller, so if I need to use it do you think I will put code at the same module to scan buttons?

[m-chichikalov]

I also do not like that we injecting everything into module namespace just by including uhsdr_board.h this is not right, I would prefer to split it and include only realy needed dependencies into each module it would help to writte more clean code and thinking about what are you doing.

[m-chichikalov]

Another side nout:
As long as we have uhsdr_types.h let's use them everywhere.
for example uint insted of uint32_t as it defined into uhsdr_types.h

It would help to play with types, for example as long as we do not care about size of program for F7 or H7 I would suggest to change define in this file from

#ifndef ushort
typedef	unsigned short	ushort;
#endif

to

#ifndef ushort
typedef	uint_fast16_t	ushort;
#endif

And so on...

[db4ple]

Max, we are trying to use uint16_t / int16_t / int8_t and not ushort etc. Why: For many not so experienced C programmers it is difficult to judge what bit count a data type has, so using a data type with an explicitly stated bit width makes in many places sense (not everywhere). I see this part of uhsdr_types.h largely as a legacy. And I also now became aware that this is not documented anywhere.

And I don't understand what you mean by "playing with types".

And finally, I understand your dislike of much what you see in the source code. I think, I mentioned that before, this is a project with a long history of different contributors and you can see the traces of this mixed history. A lot of this software is not how you would write the stuff, if you would do it "right" as a Software Engineer. But you have to give the software the credit it deserves, it does work. And this is not by accident.

However, I think, most developers would agree we should work to make the software better, both in functionality and in structure. There is no doubt about this, and quite a group of developers did this over the last couple of years.

Now, if you want to contribute here, you need the other developers to be happy with these changes and to "like" the changes, so that they like to work with them.

Now, imagine, someone else invested days of work in some changes and then has trouble merging with 61 suddenly changed files and understanding the impact of the changes on their code. And not everyone is as well trained on using git/GitHub as you probably are.

It is simply much easier for everyone if the changes come in smaller packages, step by step. Helps to build confidence and also grows your understanding of how things work in the UHSDR firmware development.

I can see in your commits at least a couple of changes you could have already pushed for the benefit of all of us.
Side note: For this kind of scenario I typically use different branches derived from the current active-devel, one for the "small changes" which can quickly pushed and integrated, and one for longer running projects, which I rebase frequently to the active-devel (or to the other branch until my change is integrated).

I do appreciate your enthusiasm, your knowledge and the amount of of work you already put into this project. Let us get comfortable working together by making it easier for us oldtimers to like your changes. Keep in mind, this is not a contest. It is a hobby project for all of us. We all want to have fun here. Otherwise the project may fall apart. To prevent this is one of my goals.

On the other hand, we need more developers like you to tackle bigger task and have to keep you happy too. Not so easy. I can tell you.

BTW, most of the users of the UHSDR firmware don't care about how the software is written, as long as it works well. They judge it from the "outside". And that is fair enough, why should they care if we use semaphores or not to make thing work.