Fixed several issues in HAL_COMPLETION code

[AdrianSoundy]

Description

When doing a new version of GPIO for ESP32 i used a completion to handle the bounce timeout
Found the completions where not working at all. Timers expired nearly straight away randomly.

After further investigation found a number of issues.

The double linked list was no longer protected with locks when items were deleted and inserted.

When inserting to list (EnqueueTicks) and list was empty it referred to random memory outside list head by using ptr->EventTimeTicks on list head.

Timers ended early due no check if time actually reached when a queued timer was superseded with short wait when CLR has nothing else to do. Although comments said there should be a check. This was probably hidden before due to CPU_Sleep.

Now we don't have CPU_Sleep implemented so goes wrong all the time.
We should have a CPU_Sleep to allow more time to be given to other tasks in RTOS.
This should be implemented.

How Has This Been Tested?

I couldn't see anything else using completion in nanoframework but i know the Env28j60 driver does use it. Tested with GPIO driver which will be separate PR.

Types of changes

• Improvement (non-breaking change that improves a feature, code or algorithm)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

Signed-off-by: adriansoundy [email protected]

[nfbot]

Hi @AdrianSoundy,

I'm nanoFramework bot.
Thank you for your contribution!

A human will be reviewing it shortly. 😉

[josesimoes]

The "completions" mechanism was abandoned in the early stages of the project when the execution engine code was reviewed. The thought process behind the decision was (in short):

• nF was now assumedly relying on a RTOS running the CLR which provides the equivalent features
• the completion list ends up being "another" task that adds weight to the - already - busy execution engine

The existing "clients" of the completion list (if I recall correctly) : GPIO debounce, I2C transactions (don't exist anymore) and some other network where replaced with RTOS features.

Any code still there related with completions is a leftover that has to be removed.

Is there any good reason/motive to revert this decision and have back completions?

[AdrianSoundy]

As it was still there i thought i would use it and i knew its was needed for Enc28j60 driver.
Not a problem i will remove the whole completion thing in this issue instead and then re-implement GPIO code using rtos features. :-)

[josesimoes]

Hate to see your work wasted... There is still a lot of code to be cleaned up...
Thanks for reworking what you've already done! 🙂👍

[AdrianSoundy]

Wasn't sure from your last comment.
Should i just remove all the Completion code ?
Or just leave it with these fixes so it at least works correctly.

For code like Enc28j60 driver which would be common code for all platforms it is good to have a common timer / callback api that can be used . So i think we should leave it.

[josesimoes]

This is tough call… I'm not fond of taking steps back from the work already done to alleviate the execution engine in an effort to make it lighter... But I understand that there isn't available a "standard" API to make this work through RTOS features.

Let's keep this fix and leave the completions there until we do a deeper rework on execution engine. In the mean time, let's avoid depending on those.
Please change your GPIO code to use RTOS features to handle the debounce so there is one less client of completions when it's replaced.

[josesimoes]

LGTM! (considering all the discussed above 😉 )