Skip to content

cpu/esp8266/gpio: use gpio_irq feature#10001

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
haukepetersen:fix_gpioirq_esp8266
Oct 9, 2018
Merged

cpu/esp8266/gpio: use gpio_irq feature#10001
miri64 merged 1 commit intoRIOT-OS:masterfrom
haukepetersen:fix_gpioirq_esp8266

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Sep 21, 2018

See #9992

This was referenced Sep 21, 2018
Merged
Closed
@miri64 miri64 added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Sep 27, 2018
@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 9, 2018

The binary size doesn't change with this PR, regardless if I compile with or without FEATURES_OPTIONAL += periph_gpio_irq, so I can't use the shortcut for testing I used in other PRs in this family. @gschorcht can you have a look?

@miri64 miri64 requested a review from gschorcht October 9, 2018 09:20
gschorcht
gschorcht approved these changes Oct 9, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

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

Following the test instructions in #9992 and after changing the tests/periph_gpio so that it becomes compilable without module periph_gpio_irq, I got the following results:

make BOARD=esp8266-esp-12x -C tests/periph_gpio

   text	   data	    bss	    dec	    hex	filename
  79179	   4325	   7352	  90856	  162e8	...

DISABLE_MODULE=periph_gpio_irq make BOARD=esp8266-esp-12x -C tests/periph_gpio

   text	   data	    bss	    dec	    hex	filename
  78799	   4325	   7200	  90324	  160d4	...

Reduces the code size by 380 bytes and memory size by 1352 bytes.

@miri64 The same can be observed using FEATURES_OPTIONAL += periph_gpio_irq and tests/periph_pwm.

@gschorcht gschorcht added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 9, 2018
@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 9, 2018 •
edited
Loading

Then something might be wrong with the current Docker image (alternatively, in #10008 I noticed that the description in #9992 is somewhat broken, see #10047, so maybe this causes the difference):

$ git rebase upstream/master  # required to include changes from #9981
First, rewinding head to replay your work on top of it...
Applying: cpu/esp8266/gpio: use gpio_irq feature
$ git show
commit 00d84fb61bef85d3641ec93bf12709a1edf6b2f3
Author: Hauke Petersen <hauke.petersen@fu-berlin.de>
Date:   Fri Sep 21 08:17:52 2018 +0200

    cpu/esp8266/gpio: use gpio_irq feature

diff --git a/cpu/esp8266/periph/gpio.c b/cpu/esp8266/periph/gpio.c
index 928866b..758df0c 100644
--- a/cpu/esp8266/periph/gpio.c
+++ b/cpu/esp8266/periph/gpio.c
@@ -164,6 +164,7 @@ int gpio_init(gpio_t pin, gpio_mode_t mode)
     return 0;
 }
 
+#ifdef MODULE_PERIPH_GPIO_IRQ
 static gpio_isr_ctx_t gpio_isr_ctx_table [GPIO_PIN_NUMOF] = { };
 static bool gpio_int_enabled_table [GPIO_PIN_NUMOF] = { };
 
@@ -225,6 +226,7 @@ void gpio_irq_disable (gpio_t pin)
 
     gpio_int_enabled_table [pin] = false;
 }
+#endif
 
 int gpio_read (gpio_t pin)
 {
$ git diff
diff --git a/tests/periph_gpio/Makefile b/tests/periph_gpio/Makefile
index 2af5396..70e6835 100644
--- a/tests/periph_gpio/Makefile
+++ b/tests/periph_gpio/Makefile
@@ -3,7 +3,7 @@ include ../Makefile.tests_common
 BOARD_INSUFFICIENT_MEMORY := arduino-duemilanove arduino-uno
 
 FEATURES_REQUIRED = periph_gpio
-FEATURES_OPTIONAL = periph_gpio_irq
+# FEATURES_OPTIONAL = periph_gpio_irq
 
 USEMODULE += shell
 USEMODULE += benchmark
$ BUILD_IN_DOCKER=1 FEATURES_OPTIONAL=periph_gpio_irq make -C tests/periph_gpio/ clean all BOARD=esp8266-esp-12x
[…]
   text	   data	    bss	    dec	    hex	filename
  78843	   4325	   7200	  90368	  16100	…
$ BUILD_IN_DOCKER=1 make -C tests/periph_gpio/ clean all BOARD=esp8266-esp-12x
[…]
   text	   data	    bss	    dec	    hex	filename
  78843	   4325	   7200	  90368	  16100	…

Note: all other platforms I tested I build without BUILD_IN_DOCKER.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 9, 2018

It's indeed docker. Somehow the FEATURES_OPTIONAL environment variable isn't provided to the docker container :-/. If I git stash I get indeed a larger image (DISABLE_MODULE passes, since I get the "expect error" output, but has also no effect on the image size):

$ BUILD_IN_DOCKER=1 make -C tests/periph_gpio/ BOARD=esp8266-esp-12x clean all
   text	   data	    bss	    dec	    hex	filename
  80303	   4325	   7352	  91980	  1674c	[…]

@miri64 miri64 mentioned this pull request Oct 9, 2018
Merged
@miri64 miri64 merged commit 328d305 into RIOT-OS:master Oct 9, 2018
@miri64 miri64 deleted the fix_gpioirq_esp8266 branch October 9, 2018 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gschorcht gschorcht gschorcht approved these changes

Assignees

No one assigned

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@haukepetersen @miri64 @gschorcht