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

Default pull mode seems different from V1, so getDigitalValue behaves differently #17

Closed
martinwork opened this issue Nov 6, 2020 · 15 comments

Comments

@martinwork
Copy link
Collaborator

By default, V1 P0 reads 0 until connected to 3V, while V2 P0 reads 1 until connected to GND.

C++

#include "MicroBit.h"

MicroBit uBit;

int 
main()
{
    uBit.init();

    while(1)
      uBit.display.printCharAsync( uBit.io.pin[0].getDigitalValue() ? '1' : '0');
}

MakeCode

basic.forever(function () {
    basic.showNumber(pins.digitalReadPin(DigitalPin.P0))
})

@finneyj
Copy link
Contributor

finneyj commented Nov 7, 2020

Thanks @martinwork

Yes, it looks like we're configured differently in the v2 target specification:
https://github.com/lancaster-university/codal-microbit-v2/blob/master/target.json#L26

Having that default pulldown in v1 was always a slightly contentious choice though (I remember much debate with the BBC)! This is an easy fix to align with v1, but tagging @jaustin here first just in case he's like to take this opportunity to make a breaking change or v2...

@jaustin? I presume would prefer to maintain consistency with v1 here?

@jaustin
Copy link
Collaborator

jaustin commented Nov 9, 2020

I do want to maintain consistency, so my default response is definitely "yes align with V1".

I don't think I was part of the discussions first time around, though, is there something I'm missing that we're hiding or breaking with this default? Do you strongly want to make a case to change? I can't see why changing to some other default helps, and I think changing to NO_PULL gives us floating voltages and non-determinism.

@finneyj
Copy link
Contributor

finneyj commented Nov 9, 2020

right. The argument was between determinism for kids looking at voltages on pins. An input always reads as zero when nothing is connected to it was more intuitive to them.

The counter argument was that no-pull is more standard for engineers and accesssory makers.

I also think set to PullDown.

@microbit-mark
Copy link
Collaborator

Also raised in micro:bit support #41704

This is causing confusion for users. If we are in agreement to maintain V1 compat, can this be implemented?

@finneyj
Copy link
Contributor

finneyj commented Dec 8, 2020

Hi @microbit-mark - I thought this was applied ages ago:
ed5dc98

Are you still seeing recent reports?

@martinwork
Copy link
Collaborator Author

It looks like NRF52Pin::getDigitalValue(void) assumes the pull mode bits in PIN_CNF have been set, and doesn't reset them from the pullMode member.

@martinwork
Copy link
Collaborator Author

I guess changing it could have side effects in isTouched and getPulseUs.

@finneyj
Copy link
Contributor

finneyj commented Dec 8, 2020

Hmmm... if changing the default hasn't taken, then we need to trace through carefully.

@martinwork
Copy link
Collaborator Author

If getDigitalValue() starts defaulting to PullDown, wherever getDigitalValue() occurs we need check it isn't assuming pullNone.

For what it's worth, all looks OK to me for getDigitalValue() to set the pull mode from pullMode (as in the DAL), except I'm not sure about NRF52SPI::config() and maybe NRF52Pin::getPulseUs().

@finneyj
Copy link
Contributor

finneyj commented Dec 8, 2020

Agree. It's just internal usage we need to be concerned about - outward usage would match v1. getPulse() was technically a v1 API too (we just pulled it down from MakeCode for v2), so historically should also have been using the default pull mode, right?

@microbit-carlos
Copy link
Collaborator

We had more reports of users hitting this difference in behaviour between v1 and v2. #67 was originally opened from a support ticket.

@jaustin
Copy link
Collaborator

jaustin commented Mar 3, 2021

@martinwork can you please confirm that as of the latest DAL this issue is still reproducable

@martinwork
Copy link
Collaborator Author

@jaustin Still the same with freshly cloned microbit-v2-samples.

@finneyj
Copy link
Contributor

finneyj commented Mar 16, 2021

Fixed in v0.2.25

@martinwork
Copy link
Collaborator Author

Retested... V1 and V2 behave the same.

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

No branches or pull requests

5 participants