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

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

Closed
bremoran opened this Issue Sep 15, 2016 · 15 comments

Comments

Projects
None yet
7 participants
@bremoran
Contributor

bremoran commented Sep 15, 2016

Description

  • Type: Bug
  • Priority: Minor

Bug

Target
BBC micro:bit

mbed-os sha:
7669d7f

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

This comment has been minimized.

Show comment
Hide comment
@0xc0170
Member

0xc0170 commented Sep 15, 2016

@kubaraczkowski

This comment has been minimized.

Show comment
Hide comment
@kubaraczkowski

kubaraczkowski Sep 24, 2016

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:

FRDM-KL25Z:

Silabs "Happy Gecko":

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 : lancaster-university/microbit-dal#213)

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

kubaraczkowski commented Sep 24, 2016

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:

FRDM-KL25Z:

Silabs "Happy Gecko":

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 : lancaster-university/microbit-dal#213)

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

@jaustin

This comment has been minimized.

Show comment
Hide comment
@jaustin

jaustin Sep 26, 2016

Contributor

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

Contributor

jaustin commented Sep 26, 2016

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

This comment has been minimized.

Show comment
Hide comment
@kubaraczkowski

kubaraczkowski Sep 26, 2016

Perhaps line up with the micro:bit DAL people (my issue here: lancaster-university/microbit-dal#213 (comment)).
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 :)

kubaraczkowski commented Sep 26, 2016

Perhaps line up with the micro:bit DAL people (my issue here: lancaster-university/microbit-dal#213 (comment)).
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

This comment has been minimized.

Show comment
Hide comment
@bremoran

bremoran Sep 26, 2016

Contributor

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.

Contributor

bremoran commented Sep 26, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jaustin

jaustin Sep 26, 2016

Contributor

@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.

Contributor

jaustin commented Sep 26, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@jamesadevine

jamesadevine Sep 26, 2016

Contributor

@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 😄, 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? 😉), other approaches carry more risk.

One final solution (😉): 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?).

Contributor

jamesadevine commented Sep 26, 2016

@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 😄, 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? 😉), other approaches carry more risk.

One final solution (😉): 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

This comment has been minimized.

Show comment
Hide comment
@jamesadevine

jamesadevine Sep 26, 2016

Contributor

@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?

Contributor

jamesadevine commented Sep 26, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@jaustin

jaustin Sep 27, 2016

Contributor

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.

Contributor

jaustin commented Sep 27, 2016

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

This comment has been minimized.

Show comment
Hide comment
@kubaraczkowski

kubaraczkowski Sep 27, 2016

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!

kubaraczkowski commented Sep 27, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bremoran

bremoran Sep 27, 2016

Contributor

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}.

Contributor

bremoran commented Sep 27, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jaustin

jaustin Sep 27, 2016

Contributor

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

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
#2713 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAI-qfHmZc3iA6hC3u-s6YIQzWhXgBg_ks5quN1ogaJpZM4J9rFR
.

Contributor

jaustin commented Sep 27, 2016

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

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
#2713 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAI-qfHmZc3iA6hC3u-s6YIQzWhXgBg_ks5quN1ogaJpZM4J9rFR
.

@bremoran

This comment has been minimized.

Show comment
Hide comment
@bremoran

bremoran Sep 27, 2016

Contributor

@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.

Contributor

bremoran commented Sep 27, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@jaustin

jaustin Sep 27, 2016

Contributor

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
#2713 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAI-qdlhQ-rlR3Zf-4YQEfrKYi_plUhDks5quOX1gaJpZM4J9rFR
.

Contributor

jaustin commented Sep 27, 2016

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
#2713 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAI-qdlhQ-rlR3Zf-4YQEfrKYi_plUhDks5quOX1gaJpZM4J9rFR
.

@RidaJichi

This comment has been minimized.

Show comment
Hide comment
@RidaJichi

RidaJichi Oct 13, 2016

Contributor

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)

Contributor

RidaJichi commented Oct 13, 2016

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)

RidaJichi added a commit to RidaJichi/mbed-os that referenced this issue Nov 1, 2016

Modified micro:bit pin names to mirror micro:bit edge connector
Resolved issue: ARMmbed#2713

Removed pins p{0..30} definitions
Defined pins P{0..20} as per micro:bit DAL's mappings:
(https://github.com/lancaster-university/microbit-dal/blob/master/inc/drivers/MicroBitPin.h)

Developers can now use the pin names as they appear on the mbed micro:bit pinout
in https://developer.mbed.org/platforms/Microbit/#pinout

Change-Id: I72b81dbe23b11d5ef215583adb211f364b4a5e91

adbridge added a commit that referenced this issue Nov 18, 2016

Modified micro:bit pin names to mirror micro:bit edge connector
Resolved issue: #2713

Removed pins p{0..30} definitions
Defined pins P{0..20} as per micro:bit DAL's mappings:
(https://github.com/lancaster-university/microbit-dal/blob/master/inc/drivers/MicroBitPin.h)

Developers can now use the pin names as they appear on the mbed micro:bit pinout
in https://developer.mbed.org/platforms/Microbit/#pinout

Change-Id: I72b81dbe23b11d5ef215583adb211f364b4a5e91

@sg- sg- closed this Jan 19, 2017

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