Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The Button A and B multiplexed pins P5 and P11 can't output low level as GPIO #84

Closed
CoolGuy-official opened this issue Mar 4, 2021 · 32 comments

Comments

@CoolGuy-official
Copy link

Hi Microbit expert, in V1 the Button A and B multiplexed pins P5 and P11 can work fine as GPIO. But now in V2 the Button A and B multiplexed pins P5 and P11 can't output low level as GPIO. We need to use more GPIO, so the pins P5 and P11 are needed. Could you tell us how to make workaround to make P5 and P11 work fine as GPIO? Or could you send one version firmware for V2 with P5 and P11 GPIO functionality? Looking forward to your response soon. My email is 33462270@qq.com, MP is +86 13958081784, welcome to discuss the solution, I'm one engineer before and want to find the solution together. Thanks a lot.

@microbit-mark
Copy link
Collaborator

Do you have a reproducer in code that works on V1 but not on V2 @CoolGuy-official?

Perhaps this is related to the default pin pull issue #17?

@finneyj
Copy link
Contributor

finneyj commented Mar 8, 2021

Thanks @microbit-mark @CoolGuy-official

I think this is different to #17. I think @CoolGuy-official is trying to reuse the pins on the edge connector that are connected to the micro:bit's buttons as digital GPIO outputs. This is most unusual and ingenious! I'm very surprised this worked on v1 to be honest!

The reason that this won't work on v2 is that the button sampling and debounce code now runs through the same APIs as user code (as used by makecode, micropython etc). Hence, that code will constantly be trying to move those pins back into a digital input, so that it can sense the button state... Essentially the button code and @CoolGuy-official code will be having a big tit-for-tat fight over who owns the pin...

@CoolGuy-official - I assume you don't want to support the use of button A/B whilst you are doing this?

We could change the button A/B handling code to be on demand activated (like most other drivers in codal). I never bothered with this before though, as there never seemed to be a point. If we do this, the button code will only start to read the pin when a user does something with the button (e.g. attempts to read it's state, creates an onButtonA block etc etc). This would be a change to a VERY well trodden codepath used by millions of people though.

@microbit-mark @microbit-carlos @jaustin - do you want to see this changed?

@microbit-mark
Copy link
Collaborator

microbit-mark commented Mar 8, 2021

I expect this relates to the hardware accessory in https://github.com/CoolGuy-official/pxt-coolguy which uses P5 and P11 for a motor accessory.

@finneyj
Copy link
Contributor

finneyj commented Mar 8, 2021

Ah ok - thanks @microbit-mark. If we have hardware out there dependent on this feature that worked on v1, I think we should make the changes described above. Suggest the foundation do as much regression testing on the buttons as possible before we pull through to online editors though.

@CoolGuy-official
Copy link
Author

@finneyj and @microbit-mark Great thanks for your support. Yes, exactly now we have P5 and P11 GPIO output senarios while button A/B are not used. I don't know if my understanding is correct. The expert @finneyj will align Button A/B code in V2 with V1, only set input when Button A/B is called. Is it right? Where can I get the this change firmware if you make this change? Thanks again.

@finneyj
Copy link
Contributor

finneyj commented Mar 9, 2021

I'll work up a version into a branch for folks to look at when I get the chance @CoolGuy-official . Once everyone is happy we can merge into the main branch and then it will automatically make its way into the MakeCode and micropython betas...

@CoolGuy-official
Copy link
Author

So happy to see that @finneyj will work on it soon. Please let us know when the branch is ready. We can do basic regression test in V2. Thanks a lot.

@martinwork
Copy link
Collaborator

Related: #53

@finneyj
Copy link
Contributor

finneyj commented Mar 13, 2021

I've now taken a look at this @CoolGuy-official @microbit-mark. See the two branches here:

https://github.com/lancaster-university/codal-microbit-v2/tree/button-demand-activation
https://github.com/lancaster-university/codal-core/tree/button-demand-activation

However, based on this, I'm now confident that we shouldn't merge these I'm afraid. As I suspected, the changes run too deep, and would change the semantics of existing APIs. To make this happen transparently, we need to do at least two things that would affect the behaviour of every user (i.e. these changes are not transparent to other users - so we're talking about millions of users here):

  • initial button sensing would not be fully debounced on the first call to Button::isPressed()
  • the result of Button::wasPressed() would be inaccurate on the first call.

Are you sure you can't just disable the buttons explicitly, and seems like the right thing to do anyway?

This works just fine in my tests against the HEAD revision of the current code:

void buttonOutputTest()
{
    uBit.buttonA.disable();
    uBit.buttonB.disable();

    while(1)
    {
        uBit.io.buttonA.setDigitalValue(1);
        uBit.io.buttonB.setDigitalValue(1);
        uBit.sleep(2000);

        uBit.io.buttonA.setDigitalValue(0);
        uBit.io.buttonB.setDigitalValue(0);
        uBit.sleep(2000);
    }
}

@CoolGuy-official
Copy link
Author

@finneyj I'm not so clear about your description and the question below.
Are you sure you can't just disable the buttons explicitly, and seems like the right thing to do anyway?

how we sue those two branches?
https://github.com/lancaster-university/codal-microbit-v2/tree/button-demand-activation
https://github.com/lancaster-university/codal-core/tree/button-demand-activation

Is it C++ code below? We use makecode to program, not C++ code below. I can understand the code below that disable button at first, and then set it as output 1 and 0 successfully, but can we program in makecode like this one?
void buttonOutputTest()
{
uBit.buttonA.disable();
uBit.buttonB.disable();

while(1)
{
    uBit.io.buttonA.setDigitalValue(1);
    uBit.io.buttonB.setDigitalValue(1);
    uBit.sleep(2000);

    uBit.io.buttonA.setDigitalValue(0);
    uBit.io.buttonB.setDigitalValue(0);
    uBit.sleep(2000);
}

}

@finneyj
Copy link
Contributor

finneyj commented Mar 14, 2021

Sorry @CoolGuy-official - I should have been more clear.

The ability to disable buttons is already implemented, and has been available for years. So, if it is ok for you to explicitly disable the buttons, then you don't need the new branches at all. This would be the best solution.

MakeCode uses these codal c++ functions for the micro:bit. So if you can call these two functions in your extension, you should be able to use the buttonA and buttonB pins (P5 and P11) as outputs.

uBit.buttonA.disable();
uBit.buttonB.disable();

You can add c++ calls like this into your MakeCode extension, but it's a good idea to just use TypeScript where possible. I don't know if MakeCode provide TypeScript versions of these functions though.

@abchatra Do you know if the API above is surfaced in TypeScript? or if there could be plans to in order to avoid the need for c++ on @CoolGuy-official's extension?

@CoolGuy-official
Copy link
Author

@finneyj thanks for your quick response even in weekend. Yes, it's great. Now I can understand it. If TypeScript for buttion disable and enable is possible, that will be more powerful to configure dynamically by user. Let's wait @abchatra 's response. Thank your help very much.

@finneyj
Copy link
Contributor

finneyj commented Mar 14, 2021

You're welcome @CoolGuy-official.

I noticed that Cool Maker is related to Zhejiang University... and Universities like to help each other. :)

Good luck with your work!

@CoolGuy-official
Copy link
Author

Yes, I graduated from Zhejiang University and now do University cooperation too with Zhejiang University to develop some new products. My e-mail is yucs@coolguymaker.com, we are in Hangzhou, Zhejiang province in China. Welcome to contact me or Zhejiang University when you visit China. I ever worked in Nokia for 16 years, have lots of good Euro colleagues and keep in touch too. So happy to get @finneyj 's help and know you well.

@CoolGuy-official
Copy link
Author

@finneyj Could you give us the guide where to add those two C++ sentence in my extension? Could you give me one example? we want to refer the bluetooth example in the pxt folder libs. But we don't understand it well. Due to we don't solve it for long time, could you help guide us ASAP? Thank you very much.

MakeCode uses these codal c++ functions for the micro:bit. So if you can call these two functions in your extension, you should be able to use the buttonA and buttonB pins (P5 and P11) as outputs.

uBit.buttonA.disable();
uBit.buttonB.disable();

@CoolGuy-official
Copy link
Author

@finneyj Could you give us the guide where to add those two C++ sentence in my extension? Could you give me one example? we want to refer the bluetooth example in the pxt folder libs. But we don't understand it well. Due to we don't solve it for long time, could you help guide us ASAP? Thank you very much.

MakeCode uses these codal c++ functions for the micro:bit. So if you can call these two functions in your extension, you should be able to use the buttonA and buttonB pins (P5 and P11) as outputs.

uBit.buttonA.disable();
uBit.buttonB.disable();

@philipphenkel
Copy link

uBit.buttonA.disable();
uBit.buttonB.disable();

@finneyj I tried to disable the buttons via C++. Unfortunately this results in a compilation error because the disable function does not seem to be available.
error: 'class MicroBitButton' has no member named 'disable

Neither the declaration of MicroBitButton at https://github.com/lancaster-university/codal-microbit-v2/blob/master/inc/compat/MicroBitButton.h nor the declaration of Button at https://github.com/lancaster-university/codal-core/blob/master/inc/drivers/Button.h contains disable()

Could you please provide a hint to me?

@finneyj
Copy link
Contributor

finneyj commented Jul 25, 2021

Hi @philipphenkel

It's declared in AbstractButton (a superclass of Button), so should work as intended: https://github.com/lancaster-university/codal-core/blob/master/inc/driver-models/AbstractButton.h

The enable()/disable() APIs were only introduced for the microbit-v2 device though. This API wasn't introduced in v1... Are you trying to do this as a MakeCode extension? If so, it's likely building for both v1 and v2. From previous discussions above, it sounds like this API is only needed for v2, so perhaps a #ifdef such that your code is only compiled for v2 devices?

@philipphenkel
Copy link

@finneyj Thanks for the additional details and the hint to the conditional compilation.

I'm using the following code in an extension and now our MakerBit board works well with both v1 and v2.

void disableButtons() {
#if MICROBIT_CODAL
    // V2 only
    uBit.buttonA.disable();
    uBit.buttonB.disable();
#endif
}

@CoolGuy-official
Copy link
Author

@philipphenkel Could you share your MakerBit board extension code link? We can't solve this issue so far. Great thanks for your help in advance. My email is yucs@coolguymaker.com.

@philipphenkel
Copy link

philipphenkel commented Jul 26, 2021

@CoolGuy-official Our pxt-makerbit-pins extension is using it to enable the pins 5 to 16 for GPIO. Please have a look at buttons.ts and buttons.cpp.

Edit: I updated the links to buttons.ts and buttons.cpp because I move the code to https://github.com/philipphenkel/pxt-buttons

@CoolGuy-official
Copy link
Author

Hi, @philipphenkel thanks for your code a lot. I can understand the implementation logic now. I can't understand namespace change reason from makerbit, button to input well.

@CoolGuy-official
Copy link
Author

Hi, @philipphenkel I add your extension. It's cool. My motor can work with pin 5 and pin 11 output. But there is one error when downloading as following. Do you know what's wrong in it.
error: pxsim.input.disableButtons is not a function.

@philipphenkel
Copy link

Hi @CoolGuy-official,
The simulator compilation issue is the reason why I'm experimenting. If you type something in the JavaScript editor, the error disappears. Therefore I assume it's a bug in the editor - a race condition in the initialization. Disclaimer: I've not done intensive testing yet.
As a next step, I will move the button code into its own extension and hope that this fixes the compilation issue.

@finneyj
Copy link
Contributor

finneyj commented Jul 26, 2021

Sounds like something worth raising on the MakeCode repo @CoolGuy-official.

@philipphenkel
Copy link

Here's my minimal button extension
https://github.com/philipphenkel/pxt-buttons

And here an example that does not compile until you add a blank line.
https://makecode.microbit.org/_2vz4zVMeVaHT

I will explore a little bit more and create an issue if I cannot resolve it.

@philipphenkel
Copy link

I created an issue at pxt-microbit microsoft/pxt-microbit#4292

@philipphenkel
Copy link

@CoolGuy-official I found a workaround to MakeCode's compilation problem.
https://github.com/philipphenkel/pxt-buttons is now fully working and can be used to disable/enable the buttons via JavaScript in MakeCode:

buttons.disable()

@CoolGuy-official
Copy link
Author

@philipphenkel that's quite cool. But there is no drag module with the functionality buttons.disable() so far. Right? If you write buttons.disable() via JavaScript, switch to grach mode, it will not work. Is it right?

@philipphenkel
Copy link

@CoolGuy-official Yes, I did not add any block descriptions. You could create an issue at https://github.com/philipphenkel/pxt-buttons, or you submit a pull request with the blocks. I would prefer the latter.

@CoolGuy-official
Copy link
Author

It's a pity that I dont' know how to submit a pull request with the block.

@microbit-carlos
Copy link
Collaborator

I'll close this as fixed, if there is still anything that needs work please feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants