search

mmurdoch / arduinounit

ArduinoUnit is a unit testing framework for Arduino libraries
MIT License
394 stars 51 forks source link

Add support for the ESP8266 env. Fix headers in readme. #57

Closed crankyoldgit closed 6 years ago

crankyoldgit commented 7 years ago

Initial support for the ESP8266 env.:

Fixes #55

readme.md:

[Tested using basic.ino & advanced.ino. They seem to run as expected.]

wmacevoy commented 7 years ago

Sorry I have not looked at this sooner. I don't see how the changes can break the standard arduino platforms, but I cant test these on the ESP. Are you willing to support the ESP changes going forward? If so, merging seems like a good idea.

crankyoldgit commented 7 years ago

I'm not going to offer great support, but I can but try.

On Fri., 28 Jul. 2017, 12:41 am Warren MacEvoy, notifications@github.com wrote:

Sorry I have not looked at this sooner. I don't see how the changes can break the standard arduino platforms, but I cant test these on the ESP. Are you willing to support the ESP changes going forward? If so, merging seems like a good idea.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mmurdoch/arduinounit/pull/57#issuecomment-318382772, or mute the thread https://github.com/notifications/unsubscribe-auth/AMInDEV3vWteycJ-swVWodZhM9_KD3Bdks5sSKGXgaJpZM4NTmRH .

wmacevoy commented 7 years ago

Ok I have looked more carefully at your code. If yeilds are necessary frequently I think there are better places for it. Instead of adding yeild inside the isLess functions (there are a lot of them, and these have grown in other branches), define the assertOp to have the yeild instead, this is more generic and covers all the other cases. Similarly, instead of placing the yeild in resolve, put it after line 234 in the run() method, which calls the others.

Can you please see if this works? If it does it should be easier to maintain.

crankyoldgit commented 7 years ago

SGTM, I'll take a look later. Basically, the watchdog timer on the esp8266 needs to be fed frequently (at least once per second) else the board will reset. The more common/frequent the operation that does a yield() or delay() etc, the better.

wmacevoy commented 7 years ago

On second thought; maybe just strip the yield method. Having it part of every assert makes testing the watchdog hard on that platform, but add a section suggesting a yield at the end of every test. Turning off testing should have as few side effects as possible.

wmacevoy commented 7 years ago

On second thought; maybe just strip the yield method. Having it part of every assert makes testing the watchdog hard on that platform, but add a section suggesting a yield at the end of every test. Turning off testing should have as few side effects as possible.

crankyoldgit commented 7 years ago

I see your point, but that's a fairly a-typical use case. It would be by far a less likely use case. In another OS etc, you wouldn't expect to be feeding the OS watch dog etc in a unit test. Given the nature of these AVR-esque devices, we effectively are the OS.

If you wanted to test the watchdog, you could construct a test-case such that it waits the appropriate time without feeding it (e.g. 1 sec, without deliberate feeding, delay()s, or yield()s) So, its easy to still test for it.

Given the watchdog causes a device reset, it's kind of out of the scope of a simple unit test. i.e. You are not likely to catch it in an exception etc.

Perhaps a better approach would be to have it configurable. Defaulting to feeding it, as that's the desired and expected case. However, I think that is unnecessary. i.e. The unit tests themselves shouldn't be a test of the watchdog timer as a side-effect.

For a typical use case/example:

Assume the watchdog causes a reset/reboot after 1 sec (1000ms) of no feeding.

If I'm testing a function, that takes say 100ms to finish. And I happen to call it 10+ times in a unit test I'm not really testing if the an individual instance of a function call itself exceeds the watch dog timer (WDT). I'm not thinking about it if I have a number of asserts in my unit tests etc. However, if my function call does take longer than 1000ms, and with our suggested code, the WDT would trigger, and hence the test would implicitly fail. That's something we'd want to detect. So, feeding the WDT in an expect/assert call really isn't an issue, IMHO.

Having people code around a property of the OS in a unit test is just yuck IMHO. Especially as it is the majority use case.

wmacevoy commented 7 years ago

Ok this has an alternate merge #58 with an arguably partly more correct solution and supports another board. It does not add yield and there is a philosophy problem there, which is why this has taken a long time to resolve. I think there should be a yield between tests, since AU has control over running multiple tests the user cannot easily influence. But adding yields at every assert I think breaks orthogonality; something you think passes a tests actually does not work on the tested platform because it takes too long to perform.

Between this, #58 and these ideas I think there is a good update.

ssilverman commented 6 years ago

Should this line: #define typeof(x) __typeof__(x) be inside the #ifdef ESP8266 section?

crankyoldgit commented 6 years ago

@ssilverman I think you are correct. It's was too long ago for me to remember why it was outside of that block in the first place. Probably human error. Moved. However, I believe it's a moot point as I think this PR is a dead-end as the project has gone down another path for ESP support.

wmacevoy commented 6 years ago

Ok, branch iss58 includes, I think, the changes to support ESP 8266 and ESP 32. Can someone please verify that? I don't have these boards.

wmacevoy commented 6 years ago

iss58 changes the way these are implemented: Compare.h is auto-generated (in meta), and so I edited the generator for Compare instead. And since __typeof__ is more portable than typeof, I changed the references to that instead, so there is nothing different about the esp boards in that regard. Finally, I did no changes about adding yeilds. I'm still not sure about a consistent way to do this...

wmacevoy commented 6 years ago

@crankyoldgit, @ssilverman : a version of this has been pulled into the v2.3.X-alpha next release. That version adds a single yield after every test called in the loop. I hope this is a reasonable compromise.