Skip to content

Conversation

cparata
Copy link
Contributor

@cparata cparata commented Sep 11, 2020

Hi Frederic,
I noticed that attachInterrupt does not give the possibility to clear pending interrupts when it is called. I would like to add a new API, called clearPendingInterrupt, that gives the possibility to do that.
Best Regards,
Carlo

@cparata
Copy link
Contributor Author

cparata commented Sep 11, 2020

Hi Frederic,
I thought a bit about this PR and maybe it is better if I implement it with just a dedicated function called clearPendingInterrupt(pin), so in this way there are not any change in the official implementation and we have just a new API for who wants to use it.
What about this proposal?
Carlo

@fpistm
Copy link
Member

fpistm commented Sep 11, 2020

Hi Carlo,
why not anyway I have to dig in this before answer.

@fpistm fpistm added enhancement New feature or request New feature labels Sep 11, 2020
@fpistm fpistm self-requested a review September 11, 2020 12:04
@fpistm fpistm added this to the 2.0.0 milestone Sep 11, 2020
@cparata cparata force-pushed the master branch 2 times, most recently from 2f5bb5e to ef557af Compare September 11, 2020 13:19
@cparata cparata changed the title Add clear pending interrupt feature with attachInterruptWithClearPend… Add clear pending interrupt feature with new clearPendingInterrupt API Sep 11, 2020
@cparata
Copy link
Contributor Author

cparata commented Sep 11, 2020

Hi Frederic,
I updated the PR with a cleaner implementation. Let me know what do you think about it.
Carlo

Copy link
Contributor

@ABOSTM ABOSTM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR LGTM.
As far I can see it may be necessary specially when disabling/enabling interrupt, because currently when calling stm32_interrupt_disable() , IT is disabled at NVIC level, but not at EXTI level. Which means that while IT is disabled, EXTI can still raise IT (but blocked at nvic), and as soon as IT is re-enabled, IT is triggered by old event on GPIO.
An alternatively to this PR could be in stm32_interrupt_disable() to disable EXTI too.
Thus more symmetric with regards to stm32_interrupt_enable().
But it changes the current behavior.
On the other hand, this PR give more flexibility to user (to clear IT at any time).
@cparata do you think this alternative would solve your original issue?
@fpistm hat is your opinion ?

@cparata
Copy link
Contributor Author

cparata commented Oct 21, 2020

Hi @ABOSTM ,
I agree with your comment. If in the "stm32_interrupt_disable" we disable also the EXTI besides the NVIC, the call of "detachInterrupt" should work in my use case. Just to summarize the use case that I have, basically, I'm using a motor driver where I have a pin that can be used as Input to detect some interrupt errors from motor driver and as Output to enable/disable the bridge of the motor driver. The logic flow is that after enabling the bridge, the pin should be configured as Input to detect overcurrent events, etc. Then, if I want to disable the bridge, I need to detach the Interrupt and then configure the pin as Output to disable the bridge. Finally, if I enable again the bridge and configure the interrupt, as soon as I attach again the interrupt callback, I receive an old interrupt due to disable/enable calls because meantime the EXTI was still enabled (even if the NVIC was disabled). The proposal of Alexandre should be compatible with this use case.
Best Regards,
Carlo

@fpistm
Copy link
Member

fpistm commented Oct 21, 2020

Hi @ABOSTM, @cparata,

having symmetric API is probably better.

@fpistm
Copy link
Member

fpistm commented Oct 23, 2020

@cparata
Could you update the PR, please ?

@cparata
Copy link
Contributor Author

cparata commented Oct 23, 2020

Hi @fpistm , @ABOSTM ,
I can rework the PR. I'm thinking to reconfigure the GPIO as Input that should be the default settings inside the "stm32_interrupt_disable()" after the disable of NVIC. In this way the pin should come back to the startup settings. Is it fine for you? Or do you have any other idea?
Best Regards,
Carlo

@ABOSTM
Copy link
Contributor

ABOSTM commented Oct 26, 2020

Hi @cparata ,
From my point of view, disabling interrupt doesn't mean user want to restore default state, he jusrt wants to disable IT: for example he can continue in polling mode.
If user wants to change mode he should do it explicitly.

@cparata
Copy link
Contributor Author

cparata commented Oct 26, 2020

Hi @ABOSTM ,
what do you suggest in this case? I guess that if you maintain the GPIO mode as IT, the EXTI will be not disabled and you will continue to receive the interrupt on EXTI line. In fact, my proposal is to put the gpio as input that should work if you want to continue to poll the gpio state.
Best Regards,
Carlo

@ABOSTM
Copy link
Contributor

ABOSTM commented Oct 27, 2020

Hi @cparata,
I guess I understand your problem: there is no simple API to change a GPIO configuration and disable corresponding EXTI.
So do you had in mind to call 'HAL_GPIO_DeInit()' and then 'HAL_GPIO_Init()' ? And thus you need to specify the new mode, thzt you proposed to be input. By the way with this solution you also need to specify pull up/down.
Instead maybe you can directly try to disable exti thanks to exti aPI like HAL_EXTI_ClearConfigLine() or LL_EXTI_DisableIT_xx_xx()

@cparata
Copy link
Contributor Author

cparata commented Oct 27, 2020

Hi @ABOSTM ,
exactly. I'm afraid that EXTI APIs cannot be used because we did not use them for the registration of the callbacks. Anyway, with my proposal we can easily save the Pull mode before calling 'HAL_GPIO_DeInit()' and then 'HAL_GPIO_Init()', so we can call the GPIO Init with the correct Pull mode.
Best Regards,
Carlo

@ABOSTM
Copy link
Contributor

ABOSTM commented Oct 27, 2020

I'm afraid that EXTI APIs cannot be used because we did not use them for the registration of the callbacks.

We did use directly EXTI API, nevertheless HAL_GPIO_Init() configure some EXTI registers. so we could do the opposite, clear some registers ... and if HAL/LL API can simplify the job that is fine with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

abandoned No more work on this enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants