Skip to content

boards/nucleo-common: fix on-board LED macro for nucleo-f302 case#7490

Merged
smlng merged 1 commit intoRIOT-OS:masterfrom
aabadie:nucleo_f302_led_fix
Aug 24, 2017
Merged

boards/nucleo-common: fix on-board LED macro for nucleo-f302 case#7490
smlng merged 1 commit intoRIOT-OS:masterfrom
aabadie:nucleo_f302_led_fix

Conversation

@aabadie
Copy link
Contributor

@aabadie aabadie commented Aug 21, 2017

In the case of nucleo-f302, the on-board LED macro was misconfigured. This PR fixes that.

@aabadie aabadie added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Aug 21, 2017
@aabadie aabadie added this to the Release 2017.10 milestone Aug 21, 2017
@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 21, 2017
* @{
*/
#ifdef CPU_MODEL_STM32F302R8
#define LED0_GPIO GPIOB
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inconsistent naming, in other places this is called LED0_PORT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this is different from port: here it's PORT_B. This macro targets the GPIO port address directly.
I made the same confusion when working on #6991.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference to here which uses LED0_PORT not LED0_GPIO as you proposed. My point is not LEDx_GPIO vs. LEDx_PORT but rather that is should be consistently named in RIOT.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference to here

Ok, I didn't notice that there were already existing macro of this kind.
I agree with the consistence argument, will change that. For the record, I made a similar change in #7085 (here) and now that I'm looking at it again, I think it needs to be changed as well...

@aabadie aabadie force-pushed the nucleo_f302_led_fix branch from 6068f63 to 24a12da Compare August 23, 2017 07:34
@aabadie
Copy link
Contributor Author

aabadie commented Aug 24, 2017

@smlng ?

@smlng
Copy link
Member

smlng commented Aug 24, 2017

ACK and GO

@smlng smlng merged commit 238b5d6 into RIOT-OS:master Aug 24, 2017
@aabadie aabadie deleted the nucleo_f302_led_fix branch February 26, 2018 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants