BBC micro:bit PinNames.h contains pin names that do not match the micro:bit pins

[bremoran]

Description

• Type: Bug
• Priority: Minor

  Bug

Target BBC micro:bit

mbed-os sha: 7669d7f8f5669dc02a8b38747a8229b22c8b4ea0

Expected behavior Setting up a pin using the pin names documented in the platform page should result in a correctly configured pin.

Actual behavior Setting up a pin with the pin names documented in the platform page will configure a different pin.

NOTE: This is not a documentation error. The micro:bit pin naming is fixed and widely documented.

Steps to reproduce

• Connect an analog sensor to the BBC micro:bit pin, marked 0.

• Execute the following code:

  #include "mbed.h"
  
  int main() {
  AnalogIn mySensor(p0);
  while (true)
  {
    printf("%f\r\n", mySensor.read());
  }
  return 0;
  }

• The code will fail to run because P0 refers to nrf51822 pin p0_0, which does not support Analog in.
• Change the AnalogIn definition above to: AnalogIn mySensor(P0_3);
• The micro:bit will now print values measured on pin 0.

\ Suggested Fix ** The micro:bit DAL contains a file that has a map of nrf51822 pins to micro:bit pins. This mapping should be added to PinNames.h.

[0xc0170]

cc @pan- @jaustin

[kubaraczkowski]

Please note that this is really confusing, especially for new users of this platform.

MBED pinouts are very clear - for every target there is a clear image on the platform page that maps precisely to what is usable in the code.

Examples: LPC1768:

• platform page (with pin mapping image)
• PinNames.h

FRDM-KL25Z:

• platform page (with pin mapping image)
• PinNames.h

Silabs "Happy Gecko":

• plaform page (with pin mapping image)
• PinNames.h

Very clear every time. Though the naming conventions are different per platform/manufacturer, they are very easy to use thanks to the clear platform page.

For Micro:Bit, however, it's totally confusing:

• image on the platform page uses "P0" "P1" etc. naming based on the edge connector - not defined at all
• PinNames.h : "P0_0", "P0_1", etc. naming based on pins of the Nordic chip. Defined, but because of very similar names to "P0" "P1" convention this gets confusing.
• micro:bit DAL: "MICROBIT_PIN_P0", etc. naming based on edge connector - defined only if you use the DAL. These are actually not clear in their own documentation, you need to dig into source code to find these mappings... (my related issue there : https://github.com/lancaster-university/microbit-dal/issues/213)

* Suggested Fix * Define "P0", "P1",etc. mapping in PinNames.h based on the official edge connector (and mbed platform image)

[jaustin]

This is a bug and @kubaraczkowski's write up is good. We should certainly fix this ASAP.

Question: should we use lowercase 'p0, p1' etc (I'll update the image on mbed) to create greater distinction between P0_0?

J

[kubaraczkowski]

Perhaps line up with the micro:bit DAL people (my issue here: https://github.com/lancaster-university/microbit-dal/issues/213#issuecomment-249353182). I'd think changing the diagram at this moment might be confusing even further since it has circled the whole of internet by now... The names P0_0 are only visible in PinNames.h (which not many people will open if the names on the diagram work out of the box) and in example code. Perhaps simply having it clear (in PinNames.h and documentation) that the P0 names are on the edge connector, but P0_0 is for "Port 0 pin 0" of the MCU is clear enough?

Changing the example code to use the "Px" names would make it clear (+ a line of documentation there). However, there's the blinky example (https://developer.mbed.org/teams/BBC/code/microbit_blinky/?platform=Microbit). It would need to use either the MCU convention "P0_x" or a mix, because the row signals don't come to the edge connector. Perhaps using both conventions in the same example that almost anyone using this platform will read/run is a good idea? There's already some explanation there why micro:bit needs a special blinky, if this introduces the concept of "MCU names" and "edge connector names" would perhaps be enough for many to avoid the obstacle. It would certainly have saved me a few hours of debugging :)

[bremoran]

I agree with @kubaraczkowski. The best thing to do is to add P0..20 directly to PinNames.h Anyone following the pinout will never notice the difference. The danger, however, is subtle capitalization bugs. It would be better if it were possible to remove all the lower-case pin names from PinNames.h or #undef them at the end of the file.

In the blinky example, I think the best bet is to use the PinNames.h-based ROW1..3, COLUMN1..9 pins. This would maintain the best usability, since they are directly usable and explicitly state their function.

[jaustin]

@bremoran and @kubaraczkowski that sounds like an ideal solution. I think #undef might be confusing and would prefer p0==P0 with P0_0 remaining, but not exposed by default in the examples, instead us9ing the ROW1, etc. @jamesadevine might have an opinion too, as he did the original file.

We'll document the P0_XX names elsewhere in micro:bit technical documentation so people can work out to use them if they need to.

[jamesadevine]

@jaustin when we worked on the platform definition, we knew that this would likely be a problem as the chip definitions are different from the exposed edge connector definitions.

We also considered the implications of case: p0 is not P0 etc. and we determined that if we did not offer the alternative and provided clear documentation, that would be sufficient.

I think if this were a standard mbed board the mappings hold true, but the micro:bit is not a standard board, it has an edge connecter with an inconsistent mapping to MCU pins, and the main methodology behind the device is that it is designed for kids.

As more users are coming on board from different backgrounds we are starting to see that experience with other mbed boards is different from the micro:bit experience :smile:, however a lot of other mbed boards have simpler MCU mappings.

All I think that needs to happen in this case is clearer documentation (releasing the schematic? :wink:), other approaches carry more risk.

One final solution (:wink:): recall all the micro:bits in the world and re-etch the pins to say PI0, PI1, PI2, but of course... that would be confusing to kids (what is PI?).

[jamesadevine]

@bremoran @kubaraczkowski I wanted to clarify, I am not against the addition of new pins, I was really playing a weird devils advocate. As long it's the addition of new pins, and not modification of old pins, that should be totally cool. 👍

My concern is there are too many definitions: p0, P0_0, P0

Is there some optimisation that could be done here?

[jaustin]

What about just never defining the lowercase ones, as Brendan suggested above? I think that's a neat solution. Bonus points: throw a CPP warning explaining why p{0-20} aren't there only if they're used. As there are no references to the lowercase pin names in the diagrams, and naively using code from other platforms using those (ie lowercase) pin names won't work, I think this is reasonable.

[kubaraczkowski]

I am 100% sure that I wouldn't like to hit the bug where p0 != P0. That would be almost impossible to find...

It sounds like the cleanest would be to kick out the lowercase names and introduce the upper-case ones. As the examples are using P0_x notation it's not very likely that someone has used the lowercase notation (which is of course possible and the suggestion of raising a warning by @jaustin would be a great way to fix that. That would leave only P0_0 and P0 names and the difference between them should be documented in multiple places. BTW. micro:bit is a great device!

[bremoran]

I like the idea of removing p{0-30}. It would be just as easy to define P0{0-30}. Currently, p{0-30} is directly mapped to P0{0-30}.

[jaustin]

We all like the idea. So let's make that a decision and Rida will do the changes.

• Remove p{0-30} definitions
• P0_XX will be the same as they already are
• P0 -> P20 will be defined as per the DAL's useful mappings: https://github.com/lancaster-university/microbit-dal/blob/master/inc/drivers/MicroBitPin.h
• Document the difference between P0_XX and PXX on the platform page clearly, and in Blinky

If someone can mangle the preprocessor to do it (I'm not sure it's possible

• in fact I'm pretty sure it's not, but that's always the best kind of problem) then we should throw a compile time error when someone tries to use p{0-30} (runtime not allowed :P)

J

On 27 September 2016 at 10:10, Brendan Moran notifications@github.com wrote:

I like the idea of removing p{0-30}. It would be just as easy to define P0{0-30}. Currently, p{0-30} is directly mapped to P0{0-30}.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ARMmbed/mbed-os/issues/2713#issuecomment-249809570, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI-qfHmZc3iA6hC3u-s6YIQzWhXgBg_ks5quN1ogaJpZM4J9rFR .

[bremoran]

@jaustin Throwing an error on use of p{0-30} is easy. Define them to something wrong in PinNames.h. E.g.

#define p0 "p0 is not defined for this platform"

Any attempt to use p0, which is defined as an integer, will cause a type conversion error.

[jaustin]

Yea, but the error message from the compiler is so misleading. I wanted something that would give you an #error rather than another obscure issue. (especially as the online compiler gives you little links to wiki help for errors, which will almost certainly mislead further)

J

On 27 September 2016 at 10:46, Brendan Moran notifications@github.com wrote:

@jaustin https://github.com/jaustin Throwing an error on use of p{0-30} is easy. Define them to something wrong in PinNames.h. E.g.

define p0 "p0 is not defined for this platform"

Any attempt to use p0, which is defined as an integer, will cause a type conversion error.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ARMmbed/mbed-os/issues/2713#issuecomment-249817908, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI-qdlhQ-rlR3Zf-4YQEfrKYi_plUhDks5quOX1gaJpZM4J9rFR .

[RidaJichi]

Necessary changes done in my fork: mbed-os: https://github.com/RidaJichi/mbed-os/commits/master (commits dated Oct 5, 2016) mbed-classic: https://github.com/RidaJichi/mbed-classic/commits/master (commits dated Oct 5, 2016) and are ready for merge (soon)