lancaster-university / codal-core

MIT License
11 stars 28 forks source link

`FiberLock::FiberLock(0)` sets the variable `locked` to 1 #152

Closed ken551 closed 2 years ago

ken551 commented 2 years ago

Hello.

I'm trying to make some apps on microbit v2, referring to the "microbit-v2-samples" repository.

Since updating codes in the repository itself and in the library folder, it seems that uBit.io.P0.setAnalogValue method has some problems. When executing the app, the program stops working at the line the setAnalogValue method is called. Due to this problem, my app which had no problem before updating stopped working.

After browsing codes I found that FiberLock::FiberLock() seems to be causing the problem.

Before updating, the method was as below, so the locked variable was set to false (=0) when the constructor is called.

FiberLock::FiberLock()
{
    queue = NULL;
    locked = false;
}

After updating, the same method was changed as below. Due to this, the locked variable is set to 1 when executing the app.

FiberLock::FiberLock( int initial )
{
    queue = NULL;
    locked = 1-initial;
}

FiberLock::FiberLock() : FiberLock( 0 ) {}

Because of this, in L838 of the FiberLock::wait() method, the program branches to true while it branches to false before the update.

    int l = ++locked;  //now "locked"=1 so "l" is set to 2
    target_enable_irq();

    if (l > 1)  // now branches to "true" because "l" is 2
    {

In my environment, fixing the arg of FiberLock(int) (L825 of CodalFiber.cpp) from 0 to one solved the problem. (I don't know how Fiber works, so I'm not sure if this is the correct fixing)

Here's my environment. Windows 11 cmake v3.23.1 ninja v1.5.3 arm-none-eabi-g++ v10.3.1 20210824

Could you check if this fixing is the correct solution?

Thank you.

finneyj commented 2 years ago

Hello @ken551

Many thanks for the detailed bug report!

Looks like a recent change causing this - @JohnVidler, Looks like a patch you made a few weeks ago has caused this issue. Can you take a look please?

JohnVidler commented 2 years ago

Hello folks,

Yup, my bad on that one, and its a good 'ol off-by-one error when I pushed the changes. I've already got a patch in the wings pending merge for it (See https://github.com/lancaster-university/codal-core/compare/master...feature/enhanced-lock ) which is due in today, so this should be a non-issue very shortly.

Sorry for the confusion - had a bit of a rush of merges before I was on leave for two weeks and I missed this :)

Will close this issue once the patch gets merged (today)

finneyj commented 2 years ago

Cool - thanks @JohnVidler @ken551!

My favourite kind of bug... one that's already been squashed. :)

JohnVidler commented 2 years ago

Merged in with this PR: https://github.com/lancaster-university/codal-core/pull/153

JohnVidler commented 2 years ago

@ken551 Thanks for reporting this - if you're working with development sources (ie using the latest master/main for all components) then if you python3 ./build.py --update you should get this fix, otherwise it'll be attached to the next tag (0.2.42)

ken551 commented 2 years ago

@finneyj @JohnVidler Thanks for your quick response. Now I updated the library and confirmed that it works without any problems. Thank you for updating (and sorry not for checking the PRs before I send this issue)