driver:adc: adc16 dma support async and repeat sample #56120
Conversation
zephyrbot
requested review from
danieldegrasse,
decsny,
DerekSnell,
dleach02,
EmilioCBen,
manuargue,
mmahadevan108 and
yvanderv
hakehuang
requested review from
teburd,
dleach02 and
nashif
and removed request for
manuargue,
yvanderv,
mmahadevan108,
dleach02,
danieldegrasse,
DerekSnell,
EmilioCBen and
decsny
|
@heinwessels , this is my test result log |
zephyrbot
requested review from
danieldegrasse,
decsny,
DerekSnell,
EmilioCBen and
manuargue
drivers/adc/adc_mcux_adc16.c
Outdated
| @@ -62,6 +62,7 @@ struct mcux_adc16_data { | |||
| #ifdef CONFIG_ADC_MCUX_ADC16_ENABLE_EDMA | |||
| const struct device *dev_dma; | |||
| struct adc_edma_config adc_dma_config; | |||
| bool batch_mode; /* batch conversion mode */ | |||
There was a problem hiding this comment.
Where is batch_mode set? And what's the difference between this and continuous_convert?
There was a problem hiding this comment.
@heinwessels , this is leaved for your RFC get merge, and I will change accordingly. the continuous_convert in NXP hardware means it will continues convert sample so the adc data register will keep updated, until adc abort this. which is a pure hardware behavour. this is why I want to seperate this from your RFC's continues, see #55783 (comment)
There was a problem hiding this comment.
Ah, I understand. It's not 100% certain if it will get added to the API though, and even if will still be called batch_mode. But I guess you can always change it in that case.
drivers/adc/adc_mcux_adc16.c
Outdated
| data->adc_dma_config.dma_block.block_size = ctx->sequence.buffer_size; | ||
| data->adc_dma_config.dma_block.block_size = 2; | ||
| if (data->batch_mode) { | ||
| data->adc_dma_config.dma_block.block_size = ctx->sequence.buffer_size; |
There was a problem hiding this comment.
Not sure how batch_mode works, but should it maybe depend on extra_samples, for example:
| data->adc_dma_config.dma_block.block_size = ctx->sequence.buffer_size; | |
| data->adc_dma_config.dma_block.block_size *= 1 + data->ctx.options.extra_samplings; |
There was a problem hiding this comment.
@heinwessels , I check your RFC #55783 , which says in continues mode it will convert till buffer is full, so I set it to the whole buffer size
There was a problem hiding this comment.
Ah, okay, but it seems I wasn't clear in the RFC, because that's not what I meant :)
- It will always set the DMA amount to transfer to
number_of_channels * (1 + extra_samplings), regardless of how big the given buffer is. (It does make sure it's big enough). - In the case of the STM32 the end of the read will be when the DMA Buffer Full Interrupt is raised. This has nothing to do with the actual buffer size.
And simply setting it to buffer_size will not allways work. What if the user specifies 2 channels with extra_samples = 1. That will result in 2 * (1 + 1) * sizeof(int16_t) = 8.
- What if the user supplied a
buffer_sizeis smaller than8?- STM32 driver throws an error in this case.
- What if the user supplied a
buffer_sizeis bigger than8? What if it's bigger by2(meaning10)? That's not enough enough for a entire sample of 2 channels, what should the driver do then?- STM32 driver ignores the extra given space, using the calculation I suggested. It's of course also possible to throw an error, but this will change what all the unit tests expect. The unit tests always give the same
bufferand samebuffer_size, regardless of the actual samplings expected. Therefore, I would expect the ADC driver to always ignore the extra bytes.
- STM32 driver ignores the extra given space, using the calculation I suggested. It's of course also possible to throw an error, but this will change what all the unit tests expect. The unit tests always give the same
IMO the suggestion is the safer approach.
There was a problem hiding this comment.
@heinwessels , in continues mode, my understanding is that we shall ignore the extra_sampling at all, and use as many buffer as possible, if user set buffer to small, then we can throw an exception. in none continues mode the extra_sampling will take effects, remember in the task_repeated_samplings case, if we do DMA for all required samples + extras, and the code in the adc_context will be confused, and we need skip them, why not just set skip the extra settings and use the buffer size directly, because by all means use need set the buffer size correctly. Because in the continues mode, the customer is expection to handle enough samples they want in maximum.
There was a problem hiding this comment.
because by all means use need set the buffer size correctly
This is not the case. Almost all drivers only has a check (something like check_buffer_size) to ensure the buffer is big enough. Have a search for buffer_size < in the drivers/adc/ folder, and you will see. For example:
zephyr/drivers/adc/adc_stm32.c
Lines 416 to 420 in 368f2e7
None of the other drivers force the buffer to be of the correct size. Additionally, if this is forced, then most of the current unit tests will also fail. In most of the current tests the supplied buffer is bigger than what's required, because the driver handles this correctly.
IMO it's very important that all drivers in Zephyr work the same when looking from the outside. :)
There was a problem hiding this comment.
Additionally, if this is forced, then most of the current unit tests will also fail. In most of the current tests the supplied buffer is bigger than what's required, because the driver handles this correctly.
by default, current unit test does not set the continues(batch_mode in NXP driver), they will pass, and we just need add a case to enable it once the RFC final. and for continues mode, user shall provide a correct buffer size(this can be document), the point here is for continues mode, the setting is to only set the continues flag(and leave the extra_samples to 0), and the correct buffer size. the driver will try to fill the buffer size as quick as possible, and do minimal check. if we enforce that the block_size = 2*(1+extra), and the continues flag, the adc_context need modify to handle it separately. and the extra_samples can not be used to repeated_samplings case. Would this be more confused? anyway, I think we can continue discuss with your RFC, as this is not this PR mainly for, agree?
There was a problem hiding this comment.
and for continues mode, user shall provide a correct buffer size(this can be document)
We could do this of course, but I don't think it should be different than other ways of sampling, which doesn't care that the buffer itsn't to big. So it would not be consistent.
by default, current unit test does not set the continues(batch_mode in NXP driver)
Also true of course, but when we add a test then it will fail, because other drivers will not implement it this way, because it would not be consistent with the way the rest of the driver works.
Regardless, this is debating over a feature that's not supported yet in Zephyr. It could still take any shape. This also means it's dead code at the moment. Therefore, adding support for it already doesn't really make sense IMO.
And also, I don't have a stake in NXP, because I've not used an NXP MCU, or worked on it's drivers. I just throught it might want to implement functionality in a consistent way with other drivers.
There was a problem hiding this comment.
We could do this of course, but I don't think it should be different than other ways of sampling, which doesn't care that the buffer itsn't to big. So it would not be consistent.
in your proposal, the repeated_samplings would not work, and we need skip it for continues mode. and if user do not provide big enough buffer, it will not fail from driver side, it is just application issue, and our test can be set correctly.
This also means it's dead code at the moment
it is not dead code, you can trigger it by set batch_mode = true in adc_init function. which just not open to user, but the fucntion is OK. and I do respect your concerns, let's wait the review from others, and update accordingly. Thanks a lot.
There was a problem hiding this comment.
@hakehuang I'm inclined to agree with @heinwessels here- While the user could enable batch mode by modifying the driver, that is not something exposed to the user or (more importantly, IMO) any test infrastructure in Zephyr. I appreciate you implementing this support, but I think if we want to support this feature we should do it as a separate PR (based on the commits in the RFC for the time being). What are your thoughts here?
There was a problem hiding this comment.
@danieldegrasse , I see. let me remove the batch_mode code. done
|
@anangl can you help to review this fixes? |
|
@danieldegrasse can you take a look please? |
add support for async call and repeat sample test 1. change the DMA req to 2 byte each 2. increase the buffer pre-dma 3. add protection on invalid buffer depends on: PR #56104 fixing: issue #56070 Signed-off-by: Hake Huang <[email protected]>
add support for async call and repeat sample test
depends on: PR #56104
fixing: issue #56070