Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow test framework to use cores/esp8266/Arduino.h directly #7377

Merged
merged 23 commits into from
Oct 6, 2020

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Jun 14, 2020

resolve #7359 , @d-a-v

  • Removes 'tests/host/common/Arduino.h' in favor of 'cores/esp8266/Arduino.h'
  • Clean-up math imports, comment some reasoning behind define / undef
  • Add a small example that does not build with the current master

Some minor stuff

  • Esp.h change is cosmetic, we might as well use class body instead of this weird forced inline definition
  • I also noticed malloc debug include at the end
#ifdef DEBUG_ESP_OOM
// reinclude *alloc redefinition because of <cstdlib> undefining them
// this is mandatory for allowing OOM *alloc definitions in .ino files
#include "umm_malloc/umm_malloc_cfg.h"

This does not cause any issues since mock does not set DEBUG_ESP_OOM, but it is unclear why it is outside of guards and whether it should be some place else ('core_esp8266_... .h'?)

cores/esp8266/Arduino.h Show resolved Hide resolved
cores/esp8266/Esp.h Outdated Show resolved Hide resolved
cores/esp8266/libc_replacements.cpp Show resolved Hide resolved
cores/esp8266/Arduino.h Outdated Show resolved Hide resolved
@d-a-v d-a-v self-requested a review June 14, 2020 17:13
@d-a-v d-a-v added this to the 3.0.0 milestone Jun 14, 2020
cores/esp8266/Arduino.h Outdated Show resolved Hide resolved
libraries/esp8266/examples/math/math.ino Outdated Show resolved Hide resolved
cores/esp8266/Esp.h Outdated Show resolved Hide resolved
cores/esp8266/libc_replacements.cpp Show resolved Hide resolved
@devyte
Copy link
Collaborator

devyte commented Jun 15, 2020

Fixes #5530 as well, if the abs and round changes are ok (I haven't checked in detail)

@d-a-v
Copy link
Collaborator

d-a-v commented Aug 5, 2020

from OP:

I also noticed malloc debug include at the end

This is history. Files and dependencies have changed from that time.
Maybe it can be moved in a better place like you suggest.

cores/esp8266/Arduino.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

While testing tests/device/test_sw_arduino_math_overrides, I had to add #include <cerrno> in tests/host/common/{litlefs,spiffs}_mock.cpp. Maybe because my host gcc version is 10.

Thanks for this work, it is always good to remove complexity and duplicate code !

@dok-net
Copy link
Contributor

dok-net commented Aug 5, 2020

In libraries/ESP8266WiFi/src/BearSSLHelpers.cpp, libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp, libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp, tests/device/test_sw_http_client/test_sw_http_client.ino there are

#if !CORE_MOCK

instead of the (canonical)

#ifndef CORE_MOCK

Could this be fixed/streamlined in the course of this PR as well?

@mcspr
Copy link
Collaborator Author

mcspr commented Aug 6, 2020

While testing tests/device/test_sw_arduino_math_overrides, I had to add #include in tests/host/common/{litlefs,spiffs}_mock.cpp. Maybe because my host gcc version is 10.

will update, seeing this too (was jumping between gcc-9 and gcc-10 machines...)

common/littlefs_mock.cpp:34:1: note: ‘errno’ is defined in header ‘<cerrno>’; did you forget to ‘#include <cerrno>’?
   33 | #include <unistd.h>
  +++ |+#include <cerrno>
   34 | #include "flash_hal_mock.h"
...
common/spiffs_mock.cpp:29:1: note: ‘errno’ is defined in header ‘<cerrno>’; did you forget to ‘#include <cerrno>’?
   28 | #include <unistd.h>
  +++ |+#include <cerrno>
   29 |
...

In libraries/ESP8266WiFi/src/BearSSLHelpers.cpp, libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp, libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp, tests/device/test_sw_http_client/test_sw_http_client.ino there are

#if !CORE_MOCK

instead of the (canonical)

#ifndef CORE_MOCK

Could this be fixed/streamlined in the course of this PR as well?

🤷
this would also require changing mock.h to a plain #define CORE_MOCK so that does not happen in the future
besides, that that is only cosmetic and might as well be added after?

bearssl(helpers|client) needs to change
littlefs comment is definitely wrong
http_client test does not need ifndef'ed block at all because axtls is removed

cores/esp8266/Arduino.h Outdated Show resolved Hide resolved
@d-a-v
Copy link
Collaborator

d-a-v commented Aug 19, 2020

Minimum for -std=c17 is 8.x.0, but my understanding is we would need to call it via gcc-{8,9}, same as g++-{7,8,9}

It seems gcc-8 is bundled with ubuntu-latest that we use in CI.

Maybe that could be useful

diff --git a/tests/host/Makefile b/tests/host/Makefile
index 1c534356..b1785d83 100644
--- a/tests/host/Makefile
+++ b/tests/host/Makefile
@@ -11,6 +11,11 @@ DEFSYM_FS ?= -Wl,--defsym,_FS_start=0x40300000 -Wl,--defsym,_FS_end=0x411FA000 -
 
 MAKEFILE = $(word 1, $(MAKEFILE_LIST))
 
+ifeq ($(shell g++-8 -v; echo $$?),0)
+CC  = gcc-8
+CXX = g++-8
+endif
+

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 9, 2020

@mcspr do you intend to fix these conflicts ?

@mcspr
Copy link
Collaborator Author

mcspr commented Sep 9, 2020

Done.
What's holding this, anyway?

@devyte
Copy link
Collaborator

devyte commented Sep 9, 2020

Me, I need to re-review.
Don't forget to please mark resolved comments as, well, resolved :)
I expect to take a look later today.

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Apologies for taking so long!

@earlephilhower
Copy link
Collaborator

@mcspr we now have a merge conflict in Arduino that I can't fix via the Web GUI. Can you please do a manual merge and re-push so we can pull this into master?

@mcspr
Copy link
Collaborator Author

mcspr commented Oct 5, 2020

Sure, will do (as sooon as I have more than 5min at the PC...)
a3281fe#diff-32484ac1bbd2e6ea02f4b86b76c2e0a2 seems to be the only change, but I am not yet sure what that change is supposed to be doing

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 6, 2020

Merge error is indeed:

CONFLICT (modify/delete): tests/host/common/Arduino.h deleted in HEAD and modified in master. Version master of tests/host/common/Arduino.h left in tree.

@@ -270,3 +270,10 @@ extern "C" void configTime(long timezone, int daylightOffset_sec,
 #include "pins_arduino.h"
 
 #endif /* Arduino_h */
+
+#if __cplusplus
+#include <WString.h>
+#include <map>
+#define MOCKARGS 1
+extern std::map<String,String> mockArgs;
+#endif

After inspection, MOCKARGS is unused anywhere, and mockArgs is only used in ArduinoMain.cpp. These are potential helpers, unused in the core, that I added to allow mocking with parameters without recompilation in user (mocking) sketches.
I think you can ignore/remove them in your PR. They shall be shared in another emulation-specific header file.

@mcspr
Copy link
Collaborator Author

mcspr commented Oct 6, 2020

These are potential helpers, unused in the core, that I added to allow mocking with parameters without recompilation in user (mocking) sketches.

Thanks for clarifying. But, any reason why not just use {get,set}env instead?

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 6, 2020

But, any reason why not just use {get,set}env instead?

Not a single tiny one 😊
I will remove this key/value facility asap

edit1: (after your PR is merged)
edit2: (updating branch now)
edit3: I just noticed you already removed it, thanks !

@d-a-v d-a-v merged commit 36b444d into esp8266:master Oct 6, 2020
@mcspr mcspr deleted the tests/sync-arduino-h branch October 6, 2020 15:20
davisonja added a commit to davisonja/Arduino that referenced this pull request Oct 16, 2020
* master: (37 commits)
  BREAKING: Change return EEPROM.end() to bool (esp8266#7630)
  BREAKING: Change return type of channel() (esp8266#7656)
  BREAKING: Change return type of RSSI() (esp8266#7657)
  Add Copyright notice to Schedule.h (esp8266#7653)
  MDNS MultiInterface (esp8266#7636)
  BREAKING: Add Wrong Password wifi status case (esp8266#7652)
  New flash writing method with offset/memory/size alignment handling (esp8266#7514)
  Fix error when debug enabled but no port chosen (esp8266#7648)
  LEAmDNSv2: change a macro name to be independant from LEAmDNS1 (esp8266#7640)
  Allow test framework to use cores/esp8266/Arduino.h directly (esp8266#7377)
  Update OTA HTTP Server Header Information (esp8266#7633)
  Add missing sntp_init/sntp_stop (esp8266#7628)
  Use direct member initialization instead of ctr initialisation (esp8266#7558)
  Prevent rewriting Updater_Signing.h if content unchanged (esp8266#7627)
  Add WiFi Multi to readme.rst
  Remove stray axtls refs, deprecated compat funcs (esp8266#7626)
  Pull deprecated axtls link (esp8266#7624)
  Redesign ESP8266WiFiMulti.[cpp|h]
  Update README.md (esp8266#7623)
  Eliminate code duplication by template for printNumber(...)/printFloat(...).
  ...
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.

Host tests: different Arduino.h contents
5 participants