Fixes #144

matthijskooijman · 2020-08-26T11:28:53Z

This PR fixes a number of issues I was having while trying to run my code inside arduino_ci. Fixes are various, see the git commit messages for details.

Note that I originally was not running arduino_ci itself, but instead using arduino_ci's cpp/arduino code separately to allow running my firmware on a regular PC in a sort of "simulator" mode, but all of the issues I encountered were easy to reproduce in a unit test as well.

Here's a small "library" that reproduces most of the problems solved in this PR (it's really just an empty library with only unittests, but if you run arduino_ci_remote.rb --skip-compilation inside it, you'll get the errors):
testlib.zip. Full output is here: errors.txt

Some of the tests in this library might be useful to include in the TestSomething sample project (which I think is basically the unittests for arudino_ci itself, right?), but I wasn't quite sure which and how (and also, I'm already quite sidetracked off my original project by this), so I left that for now.

Also, I did not add anything to the changelog yet, since that seems like a recipe for merge conflicts. How should this be done? Just add notes to the changelog as part of the commit that makes the change and hope things are merged quickly before conflicts arise? Or do you generate a changelog periodically or at release or so?

Issues Fixed

This fixes #140, #133 and part of #89.

matthijskooijman · 2020-08-26T11:58:13Z

Hm, the CI check seems to fail with two problems:

In pgmspace.h some functions are not available, like memccpy.
Some non-nill ruby assertions that I'm note quite sure what to make of

Regarding the missing functions, it seems these are the just a bit less-than-standard functions from string.h, which is correctly included and the more standard functions like memcpy are available. Looking on my own system, I see that /usr/lib/string.h defines memccpy conditionally:

#if defined __USE_MISC || defined __USE_XOPEN || __GLIBC_USE (ISOC2X)
extern void *memccpy (void *__restrict __dest, const void *__restrict __src,
                      int __c, size_t __n)
     __THROW __nonnull ((1, 2));
#endif /* Misc || X/Open.  */

However, looking at my system, unless there are some specific macros defined, at least __USE_MISC should be defined by /usr/include/features.h (included through /usr/include/x86_64-linux-gnu/bits/libc-header-start.h). So maybe there is something weird going on on the testing system? Maybe some Windows-specific thing (same header file, different casing or something)?

I can't reproduce either problem here locally, in any case...

matthijskooijman · 2020-08-26T13:19:34Z

I just noticed this PR also fixes #133, so I added that in the commit message and PR description.

While I was pushing, I pushed an extra commit to generate a bit extra output from the failing test.

Looking more closely at the output, I think that the ruby errors are actually just test failures resulting from the compilation failure (it expects a runnable executable, but that was not produced), so there's really just one problem here.

matthijskooijman · 2020-09-29T09:31:18Z

@ianfixes, I'm not quite sure what the problem is with the CI build here. Do you happen to have a local cygwin installation you work with where you could investigate locally maybe?

ianfixes · 2020-09-29T20:41:35Z

Aah, I was waiting for this to pass the tests before reviewing. I think the link to the appveyor build got broken when I moved this repo to the "Arduino CI" github org. Can you try to restart this build by force-pushing the branch? Or push a whitespace change?

cpp/arduino/avr/pgmspace.h

ianfixes · 2020-10-06T01:57:54Z

spec/cpp_library_spec.rb

+    puts "\n\nstring.h\n\n"
+    puts File.read("c:/cygwin/usr/include/string.h")
+
+    puts "\n\n_ansi.h\n\n"
+    puts File.read("c:/cygwin/usr/include/_ansi.h")
+
+    puts "\n\nsys/config.h.h\n\n"
+    puts File.read("c:/cygwin/usr/include/sys/config.h")
+
+    puts "\n\nsys/features.h.h\n\n"
+    puts File.read("c:/cygwin/usr/include/sys/features.h")
+


I assume this is for debugging and not part of the final PR

Yup, everything in the last commit was for debugging.

ianfixes · 2020-10-06T02:04:07Z

You should absolutely add lines to the top section of the changelog -- they tend to survive rebases pretty easily and the occasional merge conflict in this file is easy to resolve (pick the existing stuff, then the local stuff...)

If I'm understanding the motivation behind this PR, it's that you were attempting to define your own min function (or similar) for a particular data type, but the definition conflicted with the macro?

ianfixes · 2020-11-04T14:32:49Z

In general I am happy with all of this if it brings us into closer alignment with the way that these functions behave (compilation-wise and runtime-wise) in official Arduino.

But this fell off my radar because the CI wasn't passing -- I assumed it was still a work in progress. Is there anything I can offer to help get this to pass its CI checks?

matthijskooijman · 2020-11-05T17:50:43Z

W00ps, didn't see all of your comments, not sure what happened there. Sorry for my late reply.

If I'm understanding the motivation behind this PR, it's that you were attempting to define your own min function (or similar) for a particular data type, but the definition conflicted with the macro?

Yup. In particular, I was including, IIRC limits.h (or maybe the boost version of that) which defines a min() method, which breaks.

But this fell off my radar because the CI wasn't passing -- I assumed it was still a work in progress. Is there anything I can offer to help get this to pass its CI checks?

If you use cygwin yourself, maybe try locally to see why compilation fails. There is some weird interaction between system headers IIRC that I couldn't figure out here.

Can you try to restart this build by force-pushing the branch? Or push a whitespace change?

Done.

matthijskooijman · 2020-11-05T17:52:09Z

(Also, feel free to merge #194 separately, I can rebase this PR on top of that after it's merged)

ianfixes · 2020-11-05T17:53:52Z

My only access to a cygwin environment is via Appveyor, but multi-platform testing is exactly why I'm using it. I'm going to try to resolve that merge conflict for you.

ianfixes · 2020-11-05T18:01:32Z

Looks like that blew up, but hopefully the output makes it sensible for you to work on

matthijskooijman · 2020-11-05T18:06:16Z

Well, it doesn't, unfortunately. It spews a bunch of errors like these:

C:/projects/arduino-ci/cpp/arduino/avr/pgmspace.h: In function 'void* memccpy_P3(void*, const void*, int, size_t)':
C:/projects/arduino-ci/cpp/arduino/avr/pgmspace.h:51:84: error: 'memccpy' was not declared in this scope
 inline void *memccpy_P3(void *dest, const void *src, int val, size_t len) { return memccpy(dest, src, val, len); }

Indicating some functions are not available, but I can't figure out why not, it looks like they should be available (see also #144 (comment)).

This did not look like an issue before, because the code used macros, so i.e. memccpy() would only be referenced (and break) only if the sketch or library actually used memmcpy_P(), but now these are functions, the replacement functions must be available always, so this breaks.

matthijskooijman · 2020-11-06T11:39:43Z

I ended up just sidestepping the issue for now: I've removed the offending functions, since they're not quite common and unlikely to actually be used in practice. This should prevent this issue from blocking the other changes, would be a pity if they became stale and out of date. At a later stage (maybe when I have some more time), we can again revisit this. I put the disabling of these functions in a separate commit, so it's easy to revert later.

I've also rebased on top of current master, which worked without conflicts.

Note that I have not tested this at all anymore (I really need to get back to some other work), but the testsuite in appveyor seems to complete succesfully now, so it probably works. You might want to do some testing before merging this, though.

misc/default.yml

ianfixes · 2020-11-07T05:11:41Z

I'll try to resolve the merge conflicts from #194

In general, my testing is 100% accomplished by the CI machines... so given that your work is passing, I'm inclined to accept it. I'll try to get to that this weekend.

ianfixes · 2020-11-07T05:24:45Z

I'm not sure if this is the place to say this, but I'm very grateful for your expertise, your time, and your enthusiasm in contributing to this project. In the past 2 months this project has sailed past a bunch of technical hurdles that seemed impossible to me earlier this year.

The SPI library contains some code to read the SPCR register to determine the byte order of 16-bit transfers, presumably because there is no official Arduino API to set it, so sketches had to rely on setting this register directly. By reading it, the SPI unittest implementation can detect how to emulate multibyte transfers. However, this register is only defined by avr/io.h when the unittest emulates an AVR platform, so this code would fail to compile on other platforms. This adds a preprocessor guard around this code, defaulting to the lsb-first, which is also the hardware and Arduino default. This fixes Arduino-CI#140.

This is not done on a regular Arduino either, so this needlessly pollutes the namespace and might cause issues in some rare cases. This also changes one testcase that uses SPI to include SPI.h, since it previously relied on Arduino.h to do so.

This adds relevant defines that identify the architecture and board currently compiled for. Most of these are usually set by the platform's platform.txt and boards.txt, except for the __AVR* defines that are set by avr-gcc internally. This only adds extra defines, except for the Arduino Due, which previously incorrectly identified as an ATmega328p. This seems to fix part of Arduino-CI#89.

When this file is included, but no MCU type (e.g. `__AVR_ATmega328p__`) is defined, a warning is generated and only a partial set of AVR-specific macros is defined. When using a non-AVR target, no warning should be generated and none of these AVR-specific macros shoudl be made available, so this only includes avr/io.h from `Godmode.h` when `__AVR__` is defined, and do not include it from `avr/pgmspace.h` at all, since it is not actually needed there. This introduces a small compatibility issue: Until recently, `__AVR__` was never defined when running unittests. All included boards now do so, but if anyone has defined custom AVR boards without defining `__AVR__` in their `.arduino-ci.yaml` file, those would no longer work. This is easy enough to fix, though, just add the `__AVR__` define.

For AVR-based unittests, this was autodetected based on the registers defined in `avr/io.h`, just like on an actual build. For non-AVR targets, `NUM_SERIAL_PORTS` would always be 0. Now, any existing define on the compiler commandline (i.e. from `.arduino-ci.yaml`) takes precedence over any autodetected value.

For AVR-targets, this value is autodetected based on the MCU type, but for other targets it must be explicitly specified.

This was missing one level of pointer indirection, making any use of these functions fail at compiletime.

Previously, functions like memcpy_P were replaced by their non-progmem version using macros. However, these macros added a :: prefix, presumably to allow e.g. memcpy_P to be used in a context where a (namespace or class) local version of memcpy is defined. Adding the :: makes sure the global version is used. However, if the actual invocation of e.g. memcpy_P also uses this prefix (e.g. `return ::memcpy_P(...)`, to disambiguate when a local version of `memcpy_P` is also defined), this results in a double prefix and compilation failure. To fix this, this commit replaces the wrapper macros with inline functions that call the non-progmem version normally. These inline functions were mostly mechanically generated from the original avr/pgmspace.h documentation, removing some AVR-specific functions that do not have a standard equivalent and adding som casts here and there (where the progmem version returns `char*` and the regular version `const char*`).

Somehow, these functions cannot be found when compiling the test build under cygwin, even though it seems they should be available. To at least allow the rest of the fixes in this series to be merged, disable these functions for now. Since these are mostly uncommon functions that are unlikely to be used in actual Arduino code, this should probably not cause much problems for now.

matthijskooijman · 2020-11-07T17:13:14Z

I'll try to resolve the merge conflicts from #194

I just rebased on top of master, dropping the one commit about math macros that was essentially just copied to #194 (with just some changes in whitespace and minor other changes), so I think this should be good again.

Note that I didn't touch the changelog yet. Not 100% sure how you want it, and also no time right now to dive into this, so I'll leave this for you if you don't mind.

I'm not sure if this is the place to say this, but I'm very grateful for your expertise, your time, and your enthusiasm in contributing to this project. In the past 2 months this project has sailed past a bunch of technical hurdles that seemed impossible to me earlier this year.

Thanks, appreciated. Though I have the idea I haven't been contributing much after an initial splurge of ideas (also since I'm not actually using arduino_ci right now for my own projects yet), but good to hear that my efforts make a difference :-)

cpp/arduino/avr/pgmspace.h

ianfixes · 2020-11-11T03:36:40Z

This is in good shape to merge, and to solve a bunch of problems at once I picked it into my #183 branch

matthijskooijman force-pushed the fixes branch from 5fce911 to 607523a Compare August 26, 2020 13:17

matthijskooijman force-pushed the fixes branch 5 times, most recently from 2047a1a to da5a551 Compare August 26, 2020 15:15

matthijskooijman mentioned this pull request Aug 27, 2020

Use proper g++ options, including proper C++ std=gnu++11 #146

Open

matthijskooijman mentioned this pull request Sep 29, 2020

max() macro does not play well with std::max() #165

Closed

ianfixes reviewed Oct 6, 2020

View reviewed changes

cpp/arduino/avr/pgmspace.h Show resolved Hide resolved

ianfixes reviewed Oct 6, 2020

View reviewed changes

ianfixes mentioned this pull request Oct 6, 2020

Add Mock Support for EEPROM #174

Merged

jgfoster mentioned this pull request Nov 4, 2020

High-level wire implementation, complete with unit tests #189

Merged

jgfoster mentioned this pull request Nov 5, 2020

Prevent name clashes with math macros #194

Merged

matthijskooijman force-pushed the fixes branch from da5a551 to 3f0dab9 Compare November 5, 2020 17:50

matthijskooijman force-pushed the fixes branch 5 times, most recently from 9938db6 to 8962561 Compare November 6, 2020 11:13

matthijskooijman force-pushed the fixes branch from 8962561 to 568747b Compare November 6, 2020 11:21

ianfixes reviewed Nov 7, 2020

View reviewed changes

misc/default.yml Show resolved Hide resolved

matthijskooijman added 11 commits November 7, 2020 18:09

Show current platform while running unittests

2bc6767

Do not include SPI/Wire from Arduino.h

ec1de7e

This is not done on a regular Arduino either, so this needlessly pollutes the namespace and might cause issues in some rare cases. This also changes one testcase that uses SPI to include SPI.h, since it previously relied on Arduino.h to do so.

Specify NUM_SERIAL_PORTS for non-AVR targets

819b9e5

For AVR-targets, this value is autodetected based on the MCU type, but for other targets it must be explicitly specified.

Fix pgm_read_ptr_near/far

36b4d8c

This was missing one level of pointer indirection, making any use of these functions fail at compiletime.

Mock stdio.h progmem functions

fdfa134

matthijskooijman force-pushed the fixes branch from 568747b to a8b73e6 Compare November 7, 2020 17:11

matthijskooijman mentioned this pull request Nov 9, 2020

Possible direction for __ARDUINO_CI_SFR_MOCK #201

Closed

ianfixes mentioned this pull request Nov 10, 2020

AVR (defined as __AVR__) should not be forced for all boards #202

Closed

ianfixes reviewed Nov 10, 2020

View reviewed changes

cpp/arduino/avr/pgmspace.h Show resolved Hide resolved

matthijskooijman mentioned this pull request Nov 10, 2020

Implement missing PROGMEM / _P functions in pgmspace.h #204

Open

ianfixes closed this Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #144

Fixes #144

matthijskooijman commented Aug 26, 2020 •

edited by ianfixes

Loading

matthijskooijman commented Aug 26, 2020

matthijskooijman commented Aug 26, 2020

matthijskooijman commented Sep 29, 2020

ianfixes commented Sep 29, 2020

ianfixes Oct 6, 2020

matthijskooijman Nov 5, 2020

ianfixes commented Oct 6, 2020

ianfixes commented Nov 4, 2020

matthijskooijman commented Nov 5, 2020

matthijskooijman commented Nov 5, 2020

ianfixes commented Nov 5, 2020

ianfixes commented Nov 5, 2020

matthijskooijman commented Nov 5, 2020

matthijskooijman commented Nov 6, 2020 •

edited

Loading

ianfixes commented Nov 7, 2020

ianfixes commented Nov 7, 2020

matthijskooijman commented Nov 7, 2020

ianfixes commented Nov 11, 2020

Fixes #144

Fixes #144

Conversation

matthijskooijman commented Aug 26, 2020 • edited by ianfixes Loading

Issues Fixed

matthijskooijman commented Aug 26, 2020

matthijskooijman commented Aug 26, 2020

matthijskooijman commented Sep 29, 2020

ianfixes commented Sep 29, 2020

ianfixes Oct 6, 2020

Choose a reason for hiding this comment

matthijskooijman Nov 5, 2020

Choose a reason for hiding this comment

ianfixes commented Oct 6, 2020

ianfixes commented Nov 4, 2020

matthijskooijman commented Nov 5, 2020

matthijskooijman commented Nov 5, 2020

ianfixes commented Nov 5, 2020

ianfixes commented Nov 5, 2020

matthijskooijman commented Nov 5, 2020

matthijskooijman commented Nov 6, 2020 • edited Loading

ianfixes commented Nov 7, 2020

ianfixes commented Nov 7, 2020

matthijskooijman commented Nov 7, 2020

ianfixes commented Nov 11, 2020

matthijskooijman commented Aug 26, 2020 •

edited by ianfixes

Loading

matthijskooijman commented Nov 6, 2020 •

edited

Loading