From 7f48057b9e63b6b819954b001619058c3b6cc4f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Th=C3=A9baudeau?= Date: Mon, 6 Jan 2014 21:09:08 +0100 Subject: [PATCH 1/2] leds: Fix the API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The leds API did not work in some cases. E.g. with the following sequence: leds_off(LEDS_ALL); leds_toggle(LEDS_GREEN); leds_off(LEDS_ALL); the green LED was remaining on after the last call. This was caused by the toggle feature made synonymous with the invert feature, although it is unrelated. leds_toggle() is indeed supposed to toggle an LED, while leds_invert() is supposed to change the active level of an LED. However, all users of leds_invert() actually meant leds_toggle(), and the invert feature does not make sense in this module because it is not handy due to successive calls to leds_invert() changing the intended behavior, and hardware active levels should be managed in leds_arch_set() (e.g. by XORing the passed value with a hardware-specific constant before setting the output levels of the pins). Consequently, this change: - removes the leds_invert() function, - makes leds_toggle() behave as expected relatively to leds_off() / leds_on(), - sanitizes the code in the leds module. Signed-off-by: Benoît Thébaudeau --- core/dev/leds.c | 47 +++++++++------------ core/dev/leds.h | 3 +- cpu/avr/minileds.c | 4 -- cpu/msp430/minileds.c | 5 --- tools/sky/uip6-bridge/uip6-bridge-tap.c | 6 +-- tools/stm32w/uip6_bridge/sicslow_ethernet.c | 2 +- 6 files changed, 24 insertions(+), 43 deletions(-) diff --git a/core/dev/leds.c b/core/dev/leds.c index 1f8dcfa3438..10e7a51a9b7 100644 --- a/core/dev/leds.c +++ b/core/dev/leds.c @@ -34,54 +34,58 @@ #include "sys/clock.h" #include "sys/energest.h" -static unsigned char leds, invert; +static unsigned char leds; /*---------------------------------------------------------------------------*/ static void -show_leds(unsigned char changed) +show_leds(unsigned char new_leds) { + unsigned char changed; + changed = leds ^ new_leds; + leds = new_leds; + if(changed & LEDS_GREEN) { /* Green did change */ - if((invert ^ leds) & LEDS_GREEN) { + if(leds & LEDS_GREEN) { ENERGEST_ON(ENERGEST_TYPE_LED_GREEN); } else { ENERGEST_OFF(ENERGEST_TYPE_LED_GREEN); } } if(changed & LEDS_YELLOW) { - if((invert ^ leds) & LEDS_YELLOW) { + if(leds & LEDS_YELLOW) { ENERGEST_ON(ENERGEST_TYPE_LED_YELLOW); } else { ENERGEST_OFF(ENERGEST_TYPE_LED_YELLOW); } } if(changed & LEDS_RED) { - if((invert ^ leds) & LEDS_RED) { + if(leds & LEDS_RED) { ENERGEST_ON(ENERGEST_TYPE_LED_RED); } else { ENERGEST_OFF(ENERGEST_TYPE_LED_RED); } } - leds_arch_set(leds ^ invert); + leds_arch_set(leds); } /*---------------------------------------------------------------------------*/ void leds_init(void) { leds_arch_init(); - leds = invert = 0; + leds = 0; } /*---------------------------------------------------------------------------*/ void leds_blink(void) { - /* Blink all leds. */ - unsigned char inv; - inv = ~(leds ^ invert); - leds_invert(inv); + /* Blink all leds that were initially off. */ + unsigned char blink; + blink = ~leds; + leds_toggle(blink); clock_delay(400); - leds_invert(inv); + leds_toggle(blink); } /*---------------------------------------------------------------------------*/ unsigned char @@ -92,31 +96,18 @@ leds_get(void) { void leds_on(unsigned char ledv) { - unsigned char changed; - changed = (~leds) & ledv; - leds |= ledv; - show_leds(changed); + show_leds(leds | ledv); } /*---------------------------------------------------------------------------*/ void leds_off(unsigned char ledv) { - unsigned char changed; - changed = leds & ledv; - leds &= ~ledv; - show_leds(changed); + show_leds(leds & ~ledv); } /*---------------------------------------------------------------------------*/ void leds_toggle(unsigned char ledv) { - leds_invert(ledv); -} -/*---------------------------------------------------------------------------*/ -/* invert the invert register using the leds parameter */ -void -leds_invert(unsigned char ledv) { - invert = invert ^ ledv; - show_leds(ledv); + show_leds(leds ^ ledv); } /*---------------------------------------------------------------------------*/ diff --git a/core/dev/leds.h b/core/dev/leds.h index 82132e9a9da..333ed4288fc 100644 --- a/core/dev/leds.h +++ b/core/dev/leds.h @@ -77,13 +77,12 @@ void leds_blink(void); #endif /* LEDS_CONF_ALL */ /** - * Returns the current status of all leds (respects invert) + * Returns the current status of all leds */ unsigned char leds_get(void); void leds_on(unsigned char leds); void leds_off(unsigned char leds); void leds_toggle(unsigned char leds); -void leds_invert(unsigned char leds); /** * Leds implementation diff --git a/cpu/avr/minileds.c b/cpu/avr/minileds.c index e708e6bbfcd..70694c5dc02 100644 --- a/cpu/avr/minileds.c +++ b/cpu/avr/minileds.c @@ -58,8 +58,4 @@ leds_off(unsigned char leds) void leds_toggle(unsigned char leds) { - /* - * Synonym: void leds_invert(unsigned char leds); - */ - asm(".global leds_invert\nleds_invert:\n"); } diff --git a/cpu/msp430/minileds.c b/cpu/msp430/minileds.c index 3a2a6eabfd8..ce5f97392b7 100644 --- a/cpu/msp430/minileds.c +++ b/cpu/msp430/minileds.c @@ -74,10 +74,5 @@ leds_off(unsigned char leds) void leds_toggle(unsigned char leds) { - /* - * Synonym: void leds_invert(unsigned char leds); - */ - asm(".global leds_invert\nleds_invert:\n"); - LEDS_PxOUT ^= l2p[leds & LEDS_ALL]; } diff --git a/tools/sky/uip6-bridge/uip6-bridge-tap.c b/tools/sky/uip6-bridge/uip6-bridge-tap.c index 375c5e04562..81ba8ac8e34 100644 --- a/tools/sky/uip6-bridge/uip6-bridge-tap.c +++ b/tools/sky/uip6-bridge/uip6-bridge-tap.c @@ -64,7 +64,7 @@ tcpip_output(const uip_lladdr_t *a) /* printf("pppp o %u tx %u rx %u\n", UIP_IP_BUF->proto, packetbuf_attr(PACKETBUF_ATTR_TRANSMIT_TIME), packetbuf_attr(PACKETBUF_ATTR_LISTEN_TIME));*/ - leds_invert(LEDS_GREEN); + leds_toggle(LEDS_GREEN); } return 0; } @@ -94,7 +94,7 @@ tcpip_input(void) packetbuf_attr(PACKETBUF_ATTR_TRANSMIT_TIME), packetbuf_attr(PACKETBUF_ATTR_LISTEN_TIME));*/ slip_write(uip_buf, uip_len); - leds_invert(LEDS_RED); + leds_toggle(LEDS_RED); uip_len = 0; } } @@ -112,7 +112,7 @@ slip_tcpip_input(void) static void slip_activity(void) { - leds_invert(LEDS_BLUE); + leds_toggle(LEDS_BLUE); } /*---------------------------------------------------------------------------*/ PROCESS_THREAD(uip6_bridge, ev, data) diff --git a/tools/stm32w/uip6_bridge/sicslow_ethernet.c b/tools/stm32w/uip6_bridge/sicslow_ethernet.c index e49081bd62e..c6ac60d4109 100644 --- a/tools/stm32w/uip6_bridge/sicslow_ethernet.c +++ b/tools/stm32w/uip6_bridge/sicslow_ethernet.c @@ -968,7 +968,7 @@ void mac_802154raw(const struct radio_driver *radio) #endif slip_write(uip_buf, len); - leds_invert(LEDS_RED); + leds_toggle(LEDS_RED); //rndis_send(raw_buf, sendlen, 1); //rndis_stat.rxok++; From 984621635838b6018d23a3e2e740368322ff1bdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Th=C3=A9baudeau?= Date: Mon, 6 Jan 2014 21:53:10 +0100 Subject: [PATCH 2/2] leds: Add the leds_set() function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The leds_set() function is added on top of leds_arch_set() in order to have a means of displaying a pattern on a set of LEDs, while keeping the ENERGEST information up to date, which would be missing with a direct call to leds_arch_set(). Signed-off-by: Benoît Thébaudeau --- core/dev/leds.c | 6 ++++++ core/dev/leds.h | 1 + 2 files changed, 7 insertions(+) diff --git a/core/dev/leds.c b/core/dev/leds.c index 10e7a51a9b7..c815e100de9 100644 --- a/core/dev/leds.c +++ b/core/dev/leds.c @@ -94,6 +94,12 @@ leds_get(void) { } /*---------------------------------------------------------------------------*/ void +leds_set(unsigned char ledv) +{ + show_leds(ledv); +} +/*---------------------------------------------------------------------------*/ +void leds_on(unsigned char ledv) { show_leds(leds | ledv); diff --git a/core/dev/leds.h b/core/dev/leds.h index 333ed4288fc..73f63356cd2 100644 --- a/core/dev/leds.h +++ b/core/dev/leds.h @@ -80,6 +80,7 @@ void leds_blink(void); * Returns the current status of all leds */ unsigned char leds_get(void); +void leds_set(unsigned char leds); void leds_on(unsigned char leds); void leds_off(unsigned char leds); void leds_toggle(unsigned char leds);