Code crashing due to ADC Sync operation ( STM32F4 )

[bhardadinesh-eaton]

For ADC, We have used Sync operation for our application. Based on the timer, we are taking samples for the ADC. If we do some other operation like I2C data transfer during ADC operation. sometime, code getting crashed ( STM32F4 )

After Debugging, Came to know there is one API called adc_context_wait_for_completion() in adc_stm32.c ( zephyr-v3.0.0\zephyr\drivers\adc ) is taking some more time. Due to that, if any other operation start, It is crashing the code. Is there any suggestion, what should do to overcome this issue.

[JamesBeckley-eaton]

I'd like to elaborate more on what Dinesh is reporting. The code is making a synchronous call to the Zephyr ADC driver adc_read() in an ISR. When we run the code, everything works as expected. As soon as we stop the debugger or the code stops at a breakpoint, when the code is started again the system appears to be running, but our application code does not appear to be running. When we stop the code again, it appears that the OS is stuck in the function timeout.c::z_add_timeout() because to->dticks and t->dticks are both set to zero (see attached screen shot). As Dinesh reported, we found out that the call to adc_context_wait_for_completion() (in start_read()) is what appears to be causing the issue. If we comment out this line of code, stopping and starting the debugger works as expected. Are we allowed to make a synchronous call to adc_read() data within an ISR?

[erwango]

Converting to discussion as it sound rather as a question on ADC API.
According to the fact that adc_context_wait_for_completion is using k_sem_take(&ctx->sync, K_FOREVER);
and k_sem_take api, it seems clear that adc_ready should not be called from an ISR.
I let @anangl to comment about ADC API usage.

[JamesBeckley-eaton]

Since we can't call adc_read from an ISR, we thought we would try calling adc_read_async from an ISR. Unfortunately, this function also calls k_sem_take with a timeout of K_FOREVER, so we can't use that function from an ISR either. Is there a reason why the async read does not use calls with a no-wait timeout?

No sure why this works... but I did get adc_read_async to work from an ISR, but I had to fix something in the Zephyr driver code. It appears that adc_context_lock and adc_context_release are paired functions. Likewise, adc_context_wait_for_completion and adc_context_complete are paired functions. For an async call, adc_context_lock takes a semaphore but adc_context_release does not release it. Likewise, adc_context_wait_for_completion does not take a semaphore, but adc_context_complete does release one. Seems like these paired functions are taking and releasing semphores incorrectly for async calls. I modified adc_context_release to release the semaphore for an async call, and the code appears to work.

One other comment, if these functions adc_read and adc_read_async are not to be called from an ISR, it would be good to add a comment to the header file adc.h indicating this.

[erwango]

^^ @anangl

[anangl]

When there is no error (status == 0), adc_context_release() does not release the ctx->lock semaphore for an asynchronous operation because at that point the operation may not be finished yet. That semaphore is released in adc_context_complete() instead. This pattern was taken from spi_context.h and I agree that it may be confusing and probably should be reworked at some point, but that's really not trivial (more about this in #15703).
Both adc_read() and adc_read_async() cannot be called directly from ISRs and calls to them need to be offloaded.

[JamesBeckley-eaton]

Thank you for the quick response and explanation of how the code works. We were calling adc_read() to start the conversion because the driver does not provide another way of doing this (like a start_scan() function). We are looking for another way to do this via the STM32 HAL. If the code cannot be reworked, maybe some comments in the code explaining, for example, why the semaphore is not released would help. I also think a comment should be added to header file stating that adc_read() and adc_read_async() cannot be called from an ISR to prevent future adopters from making the same mistake we made.