Skip to content

Add weak attribute to led_set#4

Open
p3lim wants to merge 1 commit into
tmk:masterfrom
p3lim:optional_led
Open

Add weak attribute to led_set#4
p3lim wants to merge 1 commit into
tmk:masterfrom
p3lim:optional_led

Conversation

@p3lim

@p3lim p3lim commented Feb 14, 2016

Copy link
Copy Markdown
Contributor

This makes LEDs optional, lots of boards don't have any.

@tmk

tmk commented Apr 1, 2016

Copy link
Copy Markdown
Owner

It makes sense. I like to change LED api it self later but this would be useful until then.

Only adding the attribute 'weak' is enough for this pupose? don't you need default implementation somewhere?

@p3lim

p3lim commented Apr 2, 2016

Copy link
Copy Markdown
Contributor Author

It's enough, in my tests it built without errors/warnings.

@tmk

tmk commented Apr 2, 2016

Copy link
Copy Markdown
Owner

I just tested, it can compiled as you said but it stops working or run away when pressing Capslock.
From my understanding with weak attribute symbols are compiled as 0(EDIT: when no implementation is given). So if you don't have any implementation of led_set() the weak reference 0 is used when it is called. In the end it jumps into address 0 and go away.

@tmk

tmk commented Apr 2, 2016

Copy link
Copy Markdown
Owner

I guess we have two solutions;

  1. give default implementation
    In common/led.c:
__attribute__((weak))        
void led_set(uint8_t usb_led)
{
}

or use alias like: __attribute__((weak, alias("_led_set"))) ?

  1. check before calling it
    We have to check the reference everywhere led_set() is called.
diff --git a/tmk_core/common/keyboard.c b/tmk_core/common/keyboard.c
index eb7b096..bfca3e7 100644
--- a/tmk_core/common/keyboard.c
+++ b/tmk_core/common/keyboard.c
@@ -173,5 +173,5 @@ MATRIX_LOOP_END:
 void keyboard_set_leds(uint8_t leds)
 {
     if (debug_keyboard) { debug("keyboard_set_led: "); debug_hex8(leds); debug("\n"); }
-    led_set(leds);
+    if (led_set) led_set(leds);
 }

http://www.cs.virginia.edu/~wh5a/blog/The%20weak%20attribute%20of%20gcc.html

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants