[MAINTAINERS] Changes to circular callbacks

This forum is dedicated to feedback, discussions about ongoing or future developments, ideas and suggestions regarding the ChibiOS projects are welcome.
User avatar
Giovanni
Site Admin
Posts: 12098
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 549 times
Been thanked: 469 times
Contact:

[MAINTAINERS] Changes to circular callbacks

Postby Giovanni » Sun Dec 09, 2018 9:22 am

Hi,

Currently we have several drivers operating on circular buffers: ADC, DAC, I2S and SPI. Those drivers all use different conventions for callbacks, I decided to give all them the same callbacks.

I will implement the following changes:
- All drivers will have two separate callbacks, one for 1st half buffer completion, the other for 2nd half buffer completion.
- Callbacks will have no parameters except for the pointer to the driver instance.

This will improve HAL consistency and also make callbacks processing faster (no if/decisions to take in the processing path).

Suggestions? feedback?

Giovanni

faisal
Posts: 249
Joined: Wed Jul 19, 2017 12:44 am
Has thanked: 35 times
Been thanked: 32 times

Re: [MAINTAINERS] Changes to circular callbacks

Postby faisal » Mon Dec 10, 2018 9:54 pm

Thanks for addressing this!

FYI, here is a link to a post I made on this topic a while back:
viewtopic.php?t=4414#p31788

faisal
Posts: 249
Joined: Wed Jul 19, 2017 12:44 am
Has thanked: 35 times
Been thanked: 32 times

Re: [MAINTAINERS] Changes to circular callbacks

Postby faisal » Mon Dec 10, 2018 10:04 pm

If the callback will only take a parameter to the driver instance, the length of the circular buffer and the current pointer to the buffer will be stored in the driver instance struct then, right? Will there be two pointers stored in the driver struct, first half and second half? Or will be there be a single pointer to the buffer, and the user will have to do the the arithmetic to figure out the current "half"?

I'm okay with the current callback function definition which has three arguments, driver instance pointer, buffer pointer, and length.

As this is going in the HAL, how about those processors which don't have half transfer and transfer complete interrupts? Would the burden be on the HAL driver to emulate it, or were you thinking about a configuration symbol to constrain the behavior?

I hope the code for handling the circular callbacks will be made into a common library, and reused by the various drivers - instead of the current state of essentially the same code duplicated in multiple places.

Another thing to look into is the error handling code and conventions, as noted in the link I posted above to an old post I made.

User avatar
alex31
Posts: 271
Joined: Fri May 25, 2012 10:23 am
Location: toulouse, france
Has thanked: 18 times
Been thanked: 32 times
Contact:

Re: [MAINTAINERS] Changes to circular callbacks

Postby alex31 » Mon Dec 10, 2018 10:49 pm

Giovanni wrote :
I will implement the following changes:
- All drivers will have two separate callbacks, one for 1st half buffer completion, the other for 2nd half buffer completion.
- Callbacks will have no parameters except for the pointer to the driver instance.

This will improve HAL consistency and also make callbacks processing faster (no if/decisions to take in the processing path).


Hello,

Of course, improving consistency is a good thing, but i appreciate the actual implementation with one callback, i am afraid that two function definitions that will do nearly the same thing will clobber the code, probably pushing users to write a third function that does all the common work.

For the processing faster part, i am not that sure, modern compiler can do constant propagation, automatic inlining to mitigate overhead, and for the processor that have I-cache, twice the callback functions can impact performance negatively because of cache miss.

The two solutions should be benchmarked to see if there is a real advantage on the efficiency side to compensate more verbose API.

Alexandre

User avatar
Giovanni
Site Admin
Posts: 12098
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 549 times
Been thanked: 469 times
Contact:

Re: [MAINTAINERS] Changes to circular callbacks

Postby Giovanni » Tue Dec 11, 2018 9:02 am

The other option is to have a single callback then check the "state" variable to understand if it is the first half (XXX_ACTIVE) or the second half (XXX_COMPLETE). It is what the SPI driver does right now. All impacted drivers have those states so it is just matter of removing extra parameters.

About pointer to buffer and size of the data, the application, where the callback exists, already knows those:
1st half: buffer, sizeof(buffer) / 2 / sizeof(sample_t).
2nd half: buffer + sizeof(buffer) / 2 / sizeof(sample_t), sizeof(buffer) / 2 / sizeof(sample_t).

A couple of macros/inlne_functions could be added that return those values after checking the state, for example: ADC_GET_SAMPLES_PTR(adcp, buffer), ADC_GET_SAMPLES_SIZE(adcp, buffer).
This would allow to not store pointer and size into the driver structure which would take space and slow down the driver.

About the point of DMAs not supporting half transfer interrupts, usually all DMAs have that or chained blocks or scatter gather or other similar mechanisms, all could be used to implement the half buffer thing. A DMA without such features simply could not be used for circular buffers without SW tricks (2 DMA operations with SW restart ad buffer half).

Giovanni

User avatar
alex31
Posts: 271
Joined: Fri May 25, 2012 10:23 am
Location: toulouse, france
Has thanked: 18 times
Been thanked: 32 times
Contact:

Re: [MAINTAINERS] Changes to circular callbacks

Postby alex31 » Wed Dec 12, 2018 9:06 am

Ok, for me it's a good compromise.

Don't you think that this consistency campaign is a good opportunity to bring half buffer callback enhancement for the UART driver ?

Alexandre

User avatar
Giovanni
Site Admin
Posts: 12098
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 549 times
Been thanked: 469 times
Contact:

Re: [MAINTAINERS] Changes to circular callbacks

Postby Giovanni » Wed Dec 12, 2018 9:50 am

Hi,

What is the use case of circular buffers for UART driver?

Giovanni

User avatar
alex31
Posts: 271
Joined: Fri May 25, 2012 10:23 am
Location: toulouse, france
Has thanked: 18 times
Been thanked: 32 times
Contact:

Re: [MAINTAINERS] Changes to circular callbacks

Postby alex31 » Wed Dec 12, 2018 10:09 am

problem arose when trying to receive a continuous stream of data.

with the actual driver, i did not found a way to not miss bytes.

there have been a thread about this problem : exemple of using uart driver

Seems to me that a classic half buffer callback scheme should solve the problem.

For the sending part, i did not test if actual API permits ton send a stream without glitches, but if a half buffer scheme is chosen for reception, it should perhaps be symmetrical for sending.

Alexandre

User avatar
Giovanni
Site Admin
Posts: 12098
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 549 times
Been thanked: 469 times
Contact:

Re: [MAINTAINERS] Changes to circular callbacks

Postby Giovanni » Sat Dec 15, 2018 6:53 pm

Hi,

I changed ADC, DAC, I2S and SPI, now all callbacks are similar and macros are very similar, remaining differences are because intrinsic differences in drivers. I HOPE to not have introduced regressions, please report any strangeness.

About UART, I need to think about it, I am not yet convinced.

Giovanni

User avatar
alex31
Posts: 271
Joined: Fri May 25, 2012 10:23 am
Location: toulouse, france
Has thanked: 18 times
Been thanked: 32 times
Contact:

Re: [MAINTAINERS] Changes to circular callbacks

Postby alex31 » Sun Dec 16, 2018 6:43 pm

Hi,

I changed ADC, DAC, I2S and SPI, now all callbacks are similar and macros are very similar, remaining differences are because intrinsic differences in drivers. I HOPE to not have introduced regressions, please report any strangeness.


Thanks for your work !
I will have to port some of my 18.2 projects to trunk to test ...



About UART, I need to think about it, I am not yet convinced.


IMHO, actual driver make strong assumption on datagram oriented communication.

On the other side, a double buffer scheme is well adapted to bytes stream, less to datagram,
perhaps we need, for UART, a double buffer scheme with timout management that could be used on both use case.

Alexandre


Return to “Development and Feedback”

Who is online

Users browsing this forum: No registered users and 2 guests