lancaster-university / microbit-dal

http://lancaster-university.github.io/microbit-docs
Other
259 stars 131 forks source link

Should pull specification of pins be set to pull-down on first getDigitalValue call? #157

Closed cefn closed 8 years ago

cefn commented 8 years ago

This is a placeholder for the discussion around the rights and wrongs of digital inputs being rewired to add a pull-down resistor whenever they are first read (i.e. the 'what is the default' aspect of https://github.com/lancaster-university/microbit-dal/issues/156 ). As commented by @finneyj in https://github.com/bbcmicrobit/micropython/issues/288 the impact of this change would involve all the downstream languages having to agree the choice for consistency. This offers a place for the discussion to take place.

The question comes in at least two aspects.

1) Should anything be done to the digital input on getDigitalValue at all, or should the pin remain in the same wiring as if getAnalogValue had been called - (i.e. no wiring of a pull-resistor takes place). Which is the least surprising/problematic?

2) Given that an intervention is made, should it be the same wiring as if it was Button A or Button B (i.e. a pull-up). I believe kids are likely to be encouraged to extend and externalise the buttons after they have got a fun project working (e.g. if you want a fancier arcade button to pimp it up without changing the project), and so all three forms of 'pull' are likely to be encountered as part of classroom making - one if you use getDigitalValue (or attach a button to anything except ButtonA or ButtonB), a second if you use ButtonA or ButtonB and a third if you use an analog input.

dpgeorge commented 8 years ago

I think the default should be no pull.

cefn commented 8 years ago

@sparkslabs you mentioned in https://github.com/bbcmicrobit/micropython/issues/288 that having a default pull set without user overrides would certainly not be intended.

How do you feel about the choice of pull-down as an unspoken default triggered by the first invocation of digitalWrite, assuming downstream languages indeed track the change to the core API https://github.com/lancaster-university/microbit-dal/issues/156 and users therefore DO have the means of overriding the choice if they understand the issues and remember to do so.

finneyj commented 8 years ago

There is always a default - whether you set them or not. i.e. doing nothing is an implicit decision to track the default of something we don't control (e.g. mbed / nordic etc). So i think we should have a a default value.

Choices are therefore:

Benefits of PullDown are determinism, as unconnected digital inputs read as zero. This was something previously noted as being natural for kids. It also can allow for buttons/switches to be implemented with additional discrete components, and provides positive logic (button press yields a logic HI), and this is the current default (so no breaking change). As noted, existing buttons on micro:bit are PullUp though, which is an unfortunate inconsistency.

Benefits of PullNone are consistency with other APIs people may be familiar with. e.g. rPi and Arduino. Downsides are opposites of above.

Benefits of PullUp are (I think) only to make the inputs the same as a button... but changing from one interventionist default to another seems to carry a lot of negative effects. i.e. it would be a breaking change and still not align with other platforms.

I'd support either status quo or PullNone, depending how significant we deem the inconsistency against the cost of making a breaking change.

@pelikhan - an issue also of relevance for you.

pelikhan commented 8 years ago

The current behavior shipped, it's a feature now - changing it will break scripts. The lessons are written, the books are printed, etc... You would need to fix downstream languages, but also review any lessons produced on the current system, etc... My advice: no breaking changes at this point in the project.

Additionally, any change would have to be reviewed and confirmed by the BBC team in order to be percolated to microbit.co.uk.

ntoll commented 8 years ago

On 03/06/16 18:09, Peli de Halleux wrote:

in order to be percolated to microbit.co.uk

That's a lovely euphemism for all the bureaucracy involved. ;-)

N.

finneyj commented 8 years ago

Bureaucracy is more like making espresso than filter coffee. First you get ground into a fine powder, then hot steam injected through you at high pressure. What comes out is either a perfectly refined product or a puddle of bitterness.

finneyj commented 8 years ago

Default pull mode for digital input pins now configurable through config options in https://github.com/lancaster-university/microbit-dal/pull/158.

cefn commented 8 years ago

Wow, thanks for the decisive work, @finneyj

There is always a default - whether you set them or not. i.e. doing nothing is an implicit decision to track the default of something we don't control

You're absolutely right, platform defaults need to be established, but I feel that isn't what we're discussing. Note my descriptions below may be laced with straight-up mistakes from not knowing enough of the underlying capabilities or DAL design so I expect to be corrected.

I agree that on boot, pins need to be configured in either input or output mode, with high-impedance input the obvious choice here for avoidance of shorts from unused pins as consistent with Arduino. This is already in place I think.

I also agree that pins have to have some pull or another specified, Up, Down or None, and that when the platform boots probably they should all be set to some predictable and well-documented pull. This is already decided. The default pull is None I think.

I believe the design choice we are discussing isn't about choosing a default mode or pull for pins. It is is whether as an implicit by-product of a request to read the digital value on a pin without having been specific about mode and pull, at that time an additional change is made to the internal configuration and wiring of the microbit, over and above whatever mode was already set as default mode and pull. Choosing to read a pin triggers not only (inevitable) mode-setting, but also pull behaviour, as per... https://www.youtube.com/watch?v=v5hxhp0e9b4 ...so this doesn't seem to relate to platform defaults but smart interventions.

When getAnalogValue() is called after getDigitalValue, presumably the behaviour consistent with getDigitalValue would be to intervene again and disable pull-ups because it is anticipated how the pin is going to be used. Again, not about platform defaults, but about smart interventions to anticipate programmers 'mistakes'.

I am not saying this should or shouldn't be done, or what the right approach is, but I just want to be clear what we are talking about.

In fairness to the approach taken, you could say that the getDigitalValue call is getting the heat from performing a dual role here, given it intercedes and effectively makes the equivalent to an Arduino pinMode() call when digitalRead() is called without being preceded by a pinMode(). There is an arguably more-confusing thicket of interlocking behaviours and potential for underspecified behaviours between pinMode() digitalWrite() and digitalRead() on an Arduino, (digitalWrite to an pin in INPUT mode sets the pullup! what is the right result when you call digitalRead() on something configured in OUTPUT mode?) They leave this problem to the developer to fathom, and mostly just pass on the arbitrary design decisions associated with AVR registers, where by contrast the DAL is opinionated and anticipatory.

I agree with @finneyj that there is an equivalence, though. The commission of making an intervention to set pulldowns is no worse than the omission of not doing anything decisive when someone attempts to read a pin which has never been explicitly configured as an input. The pragmatics are the same; a decision whether and how to act in this underspecified case, and it has knock-on effects one way or the other.

Unfortunately, because pull settings didn't make it into any of the learner-exposed APIs at all, every getDigitalValue call is underspecified by definition, placing a lot of unfair pressure on the decision that was made for handling this single case.

jamesadevine commented 8 years ago

In tagged release v2.0.0-rc4

http://lancaster-university.github.io/microbit-docs/ubit/io/#int-getdigitalvalue-pinmode-pull

ghost commented 8 years ago

More importantly, "Puddle of Bitterness" would make a great name for a band. Or an album. Or both!

pelikhan commented 8 years ago

set pull block added to m.pxt.io (scroll to the bottom of the pins section). It's a shallow shim around the microbit-dal setPull function.

sparkslabs commented 8 years ago

Cefn, Personally, I think that Joe and Damien are in a better position to talk about a preferred default than me . My comments more related to trying to save avoiding looking somewhere not relevant, along with my reason. :-)

Personally, if any change was to occur, I would favour the change that relates to minimalising the * initial* learning curve, since it's very easy to underestimate the effect of early speed bumps. However, that's quite a big if, and l can also see good reasons for augmenting behaviour (if necessary) rather than replacing.

Any situation where you can reuse an input as both analogue or digital without explicitly switching mode will lead to a scenario that surprises someone -- in exactly the same way that floats and ints don't normally mix well. For beginners there, the default 'least surprise' is somewhat different from the least surprise for experienced developers. The same so rt of argument could apply here. Maybe :)

Yes, I know this (deliberately) dodges your question - but these sorts of considerations are different from many other systems. However, as mentioned, I'll defer to Damien and Joe here :-)

Regards

Michael On 3 Jun 2016 18:00, "Cefn Hoile" notifications@github.com wrote:

@sparkslabs https://github.com/sparkslabs you mentioned in bbcmicrobit/micropython#288 https://github.com/bbcmicrobit/micropython/issues/288 that having a default pull set without user overrides would certainly not be intended.

How do you feel about the choice of pull-down as an unspoken default triggered by the first invocation of digitalWrite, assuming downstream languages indeed track the change to the core API #156 https://github.com/lancaster-university/microbit-dal/issues/156 and users therefore DO have the means of overriding the choice if they understand the issues and remember to do so.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lancaster-university/microbit-dal/issues/157#issuecomment-223634679, or mute the thread https://github.com/notifications/unsubscribe/AAIDg_5PJWIq9i5oQY89GGOk_dWEWzA8ks5qIF3FgaJpZM4IttC8 .

finneyj commented 8 years ago

@cefn,

I don't think I agree with you here. It is just a default we're talking about here. Following the patch above:

If a pull mode is specified with setPull() it won't be overridden in getDigitalValue(). it will just activate that mode. if no mode has been explicitly set either by a previous call to setPull() or the optional parameter to getDigitalValue(), then it will go for the default, as specified through the MicroBitConfig option.

cefn commented 8 years ago

Don't get me wrong. Though I tried to articulate the problem as having two separate parts...

a) what's the default pull on platform initialisation, overriding whatever is inherited from MBed/Nordic b) whether and how any read call should opportunistically change the pull

...I think the solution that's been arrived at makes a great deal of sense and is an excellent compromise esp. given the legacy as noted by @pelikhan

whaleygeek commented 8 years ago

Just for completeness:

The original spec for the micro:bit, as published by the BBC on 06/07/2015 (nearly a year ago) is quoted as saying "Inference-based digital and analogue I/O abstractions", which is indeed what this is - inferred from the mode of use, the driver auto reconfigures things for you.

http://www.bbc.co.uk/mediacentre/mediapacks/microbit/specs

Although I note also that page also says "capacitive touch sense", so we can't believe everything on that page, as the DAL docs actually say that touch sensing is 'resistive':

http://lancaster-university.github.io/microbit-docs/ubit/io/

finneyj commented 8 years ago

thanks all. I'm closing this one off as it seems we have reached a sort of broad consensus here, and some of the more specific aspects are now being discussed in the related issues above.