Skip to content

boards: re-work LED handling#7350

Closed
haukepetersen wants to merge 3 commits intoRIOT-OS:masterfrom
haukepetersen:opt_board_ledhandling
Closed

boards: re-work LED handling#7350
haukepetersen wants to merge 3 commits intoRIOT-OS:masterfrom
haukepetersen:opt_board_ledhandling

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Jul 11, 2017

WIP/RFC

I quickly played around a bit when thinking about #7321... Comments welcome!

So far only adapted samr21-xpro and stm32f3discovery.

Code size for tests/leds:
stm32f3discovery (8 LEDs) - before 8692, after: 8200 -> saves 492 byte
'samr21-xpro` (1 LED) - before 7860, after: 7960 -> adds 100 byte

Code size for hello-world:
stm32f3discovery (8 LEDs) - before 9036, after: 9040 -> adds 4 byte
'samr21-xpro` (1 LED) - before 8620, after: 8672 -> adds 52 byte

I am not sure, but it seems to me, that the sam is not inlining correctly, need to investigate...

@haukepetersen haukepetersen added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jul 11, 2017
@haukepetersen haukepetersen mentioned this pull request Jul 11, 2017
Closed
@smlng
Copy link
Copy Markdown
Member

smlng commented Jul 11, 2017

I pretty much like that 👍 I volunteer to adapt further boards, will test soon!

@smlng
Copy link
Copy Markdown
Member

smlng commented Jul 11, 2017

btw. regarding the sizes: with my approach in #7321 I also observed an increase (>100B) in size for some boards (samr21-xpro and PhyNode) while others exhibit a rather large decrease (remote-revb) ...

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Jul 11, 2017

Nice to hear you like it! But before spending any more work on this, we should reach a common understanding about removing the old LED macros first... Maybe it makes sense to post that discussion on the devel list?!

@smlng
Copy link
Copy Markdown
Member

smlng commented Jul 11, 2017

we could also keep but redirect them to the new handlers? I mean, contrary to SPI rework this not direct replacement.

@smlng
Copy link
Copy Markdown
Member

smlng commented Jul 11, 2017

and mark their usage as deprecated and remove them in release 2018.01 (or so)

@smlng
Copy link
Copy Markdown
Member

smlng commented Jul 11, 2017

and yes, we should announce this on devel-list to get more attention - not everyone follows all the github noise.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Jul 11, 2017

Replacing the old defines gracefully makes sense. I think the key her is, that the new LED handling adds some (small) function call overhead when cotrolling the LEDs, which is a problem when using the LED pins for timing measurements etc. But if we decide this is ok, I think it would be a quick thing to adapt all boards to this PR.

@smlng
Copy link
Copy Markdown
Member

smlng commented Jul 11, 2017

as I already stated in #7321 I see the benefit in a very much simplified board configuration, as we basically get rid of all the defines for ON/OFF/TOGGLE macros - further, we get a proper API, which is one of RIOTs core idea (features). This certainly adds on size, but as it is a module every one is free to add it or not. Regarding the timing stuff, I've to admit I'm not much into that right now, but sounds like its rather a test case not a something for production?

smlng
smlng reviewed Jul 11, 2017
View reviewed changes
boards/stm32f3discovery/board.c
gpio_init(LED5_PIN, GPIO_OUT);
gpio_init(LED6_PIN, GPIO_OUT);
gpio_init(LED7_PIN, GPIO_OUT);
onboard_led_init();
Copy link
Copy Markdown
Member

@smlng smlng Jul 11, 2017

Choose a reason for hiding this comment

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

should be guarded with #ifdef MODULE_ONBOARD_LED

Copy link
Copy Markdown
Contributor Author

@haukepetersen haukepetersen Jul 11, 2017

Choose a reason for hiding this comment

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

certainly.

smlng
smlng reviewed Jul 11, 2017
View reviewed changes
boards/stm32f3discovery/Makefile.include
export CPU_MODEL = stm32f303vc

# use on-board LEDs
USEMODULE += onboard_led
Copy link
Copy Markdown
Member

@smlng smlng Jul 11, 2017

Choose a reason for hiding this comment

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

I'm unsure if we really want to make it a default per board, or rather let the developer decide on a per app/tests usage?

Copy link
Copy Markdown
Contributor Author

@haukepetersen haukepetersen Jul 11, 2017

Choose a reason for hiding this comment

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

true. I guess it does indeed make sense to only pull the module if needed.

Further thought: we could also move the call to onboard_led_init from the board initialization into the cpu arch initialization, e.g. the reset_handler_default for all cortexm based boards...

@haukepetersen haukepetersen mentioned this pull request Jul 11, 2017
Closed
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Jul 11, 2017

I am also not very happy with the name of the module, isn't a simple led more suitable?

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Jul 11, 2017

I did not have time to look at the code, but I think the module may be better named simply led, like you suggest.

@smlng
Copy link
Copy Markdown
Member

smlng commented Jul 11, 2017 •
edited
Loading

Regarding the naming, IMHO against plain and simple led speaks that [it] is certainly no generic, higher layer LED handling API, but rather what it currently says: to manipulate onboard LEDs (directly attached to GPIOs).

@aabadie aabadie modified the milestone: Release 2017.10 Jul 12, 2017
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Jul 13, 2017

oh by the way, when introducing this module we can also have the LED automatically exported via SAUL without having to define the SAUL gpio config for the LEDs manually anymore...

@aabadie aabadie modified the milestone: Release 2017.10 Jul 15, 2017
@smlng
Copy link
Copy Markdown
Member

smlng commented Aug 28, 2017

how do we proceed here? I still like this 😉

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 28, 2017

me too. As there was not response on the mailing list when I put this topic out, I guess most people don't have a strong opinion. When I understood @OlegHahm's reply correctly, then he doesn't mind to go ahead if we provide a similar alternative.

So I propose we focus on #7352 first (as this provides this alternative), and once merged we get this PR on the road.

@smlng
Copy link
Copy Markdown
Member

smlng commented Aug 28, 2017

So I propose we focus on #7352 first

fine by me, looks small & simple - maybe something for tomorrows HnA

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 28, 2017

yes, thats what I wanted to hear :-)

@smlng smlng removed this from the Release 2018.01 milestone Jan 12, 2018
@dylad
Copy link
Copy Markdown
Member

dylad commented Jul 4, 2018

Any progress on this ?

@PeterKietzmann PeterKietzmann added the State: archived State: The PR has been archived for possible future re-adaptation label Jul 5, 2018
@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Jul 5, 2018

There was no progress since one year which leads me to the conclusion that there's no explicit interest in this. Personally I think it's a nice approach, though I don't really see the need. People seem to be happy with our simple LED macros. Furthermore, code size of the test application is not critical IMO.

It's just a wild guess but probably there 'll be no efforts to adapt all platforms next to I2C rework and whatever goes on, so I'm closing this one with memo label set. @haukepetersen if you wanna reactivate and convince the community I'm willing to help anyway.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Jan 15, 2020

nope, just never found the time :-)

@haukepetersen haukepetersen reopened this Jan 15, 2020
@haukepetersen haukepetersen added State: don't stale State: Tell state-bot to ignore this issue and removed State: archived State: The PR has been archived for possible future re-adaptation labels Jan 15, 2020
@tcschmidt tcschmidt requested a review from kaspar030 April 1, 2020 01:04
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@Teufelchen1
Copy link
Copy Markdown
Contributor

Teufelchen1 commented Jan 23, 2024

@haukepetersen what is you status on this one? Still working on it?

@Teufelchen1 Teufelchen1 closed this Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaspar030 kaspar030 Awaiting requested review from kaspar030

1 more reviewer

@smlng smlng smlng left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jnohlgard jnohlgard

@kYc0o kYc0o

@smlng smlng

Labels

Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: don't stale State: Tell state-bot to ignore this issue State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@haukepetersen @smlng @jnohlgard @dylad @PeterKietzmann @Teufelchen1 @aabadie @kYc0o @MrKevinWeiss