Mock for sleep and wdt

[PRosenb]

This is my first pull request to contribute code to this project. Constructive feedback is therefore very welcome to allow the project to develop consistently.

• add avr/sleep.h with calls to GodmodeState
• add avr/wdt.h with calls to GodmodeState
• add new structs SleepDef and WdtDef
• add new structs in GodmodeState and reset them on reset()
• add interrupt define for ISR to allow the production code to specify an ISR for wdt
• rename existing ISR in function attachInterrupt() as it conflicts with AVR's ISR
• add storage location to allow usage of ADCSRA
• add tests for the new functionality

[ianfixes]

Hi @PRosenb, thank you for taking the time to contribute this! Doubly so, since I'm not familiar with the use of these features.

Related to that, my first suggestion is that you should add some comments with URLs for the API for these features and/or some examples of how they are used (so that I, as a maintainer, have an easy reference for whether the mocks match the expected usage). Whether it makes more sense to add them to the definitions of the mocks or to the examples is something I'll leave to you.

I have a few other thoughts that i'll leave in-line

[ianfixes]

Where is ADCSRA defined?

[PRosenb]

I added explanations into this file

[ianfixes]

Please move this to interrupts.cpp, and add a comment with some explanation of WDT_vect.

[PRosenb]

👍

[ianfixes]

Please add a link to the ISR() docs. Also, what is the extern doing here? What does this macro perform?

[PRosenb]

I added an explanation there

[ianfixes]

Please link to WDT docs

[PRosenb]

👍

[ianfixes]

Please link to sleep-mode docs

[PRosenb]

👍

[ianfixes]

I would like to tie the GODMODE stuff you are doing to a queue-based implementation, so that we can observe the history as well as the timing of the commands (plus, it will match what PinHistory does). That would have to wait for some work I am doing on #23. Can I delay this PR a bit while I finish that up?

[PRosenb]

Thanks for your feedback @ianfixes, I updated the PR accordingly.
When do you think to finish https://github.com/ianfixes/arduino_ci/issues/23?
Are you making PinHistory generic so that it can be use for any kind of history?

[ianfixes]

Are you making PinHistory generic so that it can be use for any kind of history?

Sort of. PinHistory wraps a pair of queues (one for data that has been set, one for incoming reads) and I will probably create an intermediate timestamp-aware queue class, which I would use here. It will take some time to sort it out, but it's a priority for me as it blocks other features I'd like to add.

[ianfixes]

I've laid the foundation for this in #116, which I will merge shortly.

With that done, I have a few comments on this.

1. Your question about PinHistory is correct -- it's a generic class designed to handle recording some history of when things were set. The _count fields are no longer necessary; for now, that task would fall to the unit test writer.
2. Move all your mocks out of avr/ (see https://github.com/ianfixes/arduino_ci/blob/master/cpp/arduino/avr/README.md -- that dir is for headers copied from the mainline project). You should be able to put all of these in Godmode.h and Godmode.cpp with the other mocks of built-ins
3. Remove AvrAdc.h / AdrAdc.cpp and make that part of one of the existing headers, preferably inlined.
4. Remove all the SampleProjects/DoSomething/examples/* you added, as they don't validate any of the code in this repo (the sketches I do have there are for validating Ruby behavior, not C++). If the code in those sketches is meant to exercise some part of your C++ mocks, the only way to do that is by converting them to unit tests.
5. All unit tests should start with #include <ArduinoUnitTests.h> / #include <Arduino.h>. Including the specific files is well-intentioned, but we need to ensure that #include <Arduino.h> is bringing in all bult-ins you would expect to access in a regular sketch.

[PRosenb]

Hi @ianfixes, thanks for working on this, looks like a lot of effort..
To you points above:
2. Where is a good location to put them? These header files need to be in an avr/ directory because the production code includes them from there e.g.
#include <avr/sleep.h>
3. I generally prefer to separate things, that's why I put ADC specific code separately. In which existing files would you like to include it?
4. Do you want to delete them? I added them for you so that you (and others that adjust this project) see how production code uses this functionality. They are not thought as test but as examples, I put tests separately. Possibly we should create an other example location for this kind of examples?
5. Good point, makes sense 👍

[ianfixes]

Aah, OK. I see from the docs (https://www.nongnu.org/avr-libc/user-manual/group__avr__sleep.html) that these files must be included. I'm a bit torn here, the entire avr/ directory is a big hack that took a while to get working -- I don't want to complicate it by making it a mix of created code and borrowed code.

I think it would be acceptable to create an empty file (well, a file with just a comment explaining this purpose) at avr/sleep.h and however many other files need to be created. Also note their existence in the cpp/arduino/avr/README.md to reduce the chances of clobbering it later. That takes care of the case where someone's library includes those files -- unit test compilation won't fail.

For the actual implementation, put anything that relies on Godmode support into Godmode.h/Godmode.cpp. There should be no harm unconditionally including those functions (and if we find some problem later, we can address it at that time). But for organization's sake, those functions need to be grouped with their backend stuff.

For stuff that doesn't need Godmode, use cpp/arduino/Arduino.h directly (instead of including from there). Or for defines, you're welcome to put them in ArduinoDefines.h, or their own file as you deem appropriate (e.g. what I do with binary.h, Serial.h, etc). If those files get too big I can address that sometime later... but my aim is to centralize the mock code as much as possible.

Yes, please delete the example sketches. The logic in there should be copied to unit tests though... You can (and should) keep what's in the loop() { }, except just run it once and assert what you expect the Godmode state to be. That will help prevent me (or some other contributor) from accidentally breaking that code in the future.

[hlovdal]

I'm a bit torn here, the entire avr/ directory is a big hack that took a while to get working -- I don't want to complicate it by making it a mix of created code and borrowed code.

One way to handle this could be to create a sub directory below avr, say arduino_ci and let the file in avr just be a wrapper, e.g. cpp/arduino/avr/sleep.h contains just

#include "arduino_ci/sleep.h"

That way it is quite clear what files are directly avr related and which files are testing frame work related (alternatively include from the parent directory).

[ianfixes]

At the moment, it looks like the available options boil down to a single choice:

• put empty files in cpp/arduino/avr/ (i.e. files that contain only comments, no code)
• put non-empty files (code) in cpp/arduino/avr/

For the non-empty option, I don't see a better way to do it than what @PRosenb is already doing (except that I'd like some explicit comments in the file and a note in cpp/arduino/avr/README.md that these files are mocks). I could live with this and fix it later if I come up with a better way.

The empty-file option is mostly the same (comments in the files and a note in the README), but it keeps the mocks with the other mocks.

I realize I'm making a big deal out of what looks like a very trivial thing, but over the course of developing this library I've had to refactor the entire mock system a few times... and it's very likely that I will have to do so again before the entire set of builtins is mocked. Keeping a very clear picture of where the mocks "live" (and confining them to only a few files) makes that job manageable. If/when we reach some level of maturity or receive some blessing from the core Arduino team for how we do this, I can relax that a bit.

[hlovdal]

I would prefer to not go for the empty file option, because that would imply that including Arduino.h becomes mandatory. For instance the file where I keep my interrupt handler includes just

#include <avr/interrupt.h>
#include <avr/io.h>
#include <avr/pgmspace.h>

because that's all what's needed. Of course I could add an include of Arduino.h and it would not be a disaster, but it feels wrong to impose constraints like that just because of some implementation detail in the test framework.

(and yes, that avr/interrupt.h implies that I had also started on doing some of this work, although by no means as thorough and complete as @PRosenb).

---

Regarding the AdrAdc.{h,cpp} files I do not mind integrating them into existing files, just want to note that if they are kept as separate files they ought to be renamed to AvrRegisters.{h,cpp} because it is not just related to the ADC, this affects all other registers defined with the _SFR_MEM8 macro.

There also exists 16 and 32 bit versions of the _SFR_MEM8 macro, and for consistency I think we should cover all of them and handle them in the same way. In addition I wanted to add a comment about the register "collision", which for my part I am perfectly happy with for now, but I want to have it explicitly documented and with some thoughts on how to possibly improve.

diff --git a/cpp/arduino/AvrAdc.cpp b/cpp/arduino/AvrAdc.cpp
index 4699e8c..190fded 100644
--- a/cpp/arduino/AvrAdc.cpp
+++ b/cpp/arduino/AvrAdc.cpp
@@ -1,4 +1,7 @@
 #include "AvrAdc.h"
 
-// mock storage to allow access to ADCSRA
-unsigned char sfr_store;
+// mock storage to allow access to ADCSRA and other registers accessed through the _SFR_MEM8/16/32 macros.
+uint8_t  sfr8_store;
+uint16_t sfr16_store;
+uint32_t sfr32_store;
+
diff --git a/cpp/arduino/AvrAdc.h b/cpp/arduino/AvrAdc.h
index f5e2cd5..26f2824 100644
--- a/cpp/arduino/AvrAdc.h
+++ b/cpp/arduino/AvrAdc.h
@@ -1,9 +1,54 @@
 #pragma once
 
+#include <stdint.h>
+
 // ADCSRA is defined in the CPU specific header files
 // like iom328p.h.
 // It is liked to _SFR_MEM8 what does not exists in the test environment.
 // Therefore we define _SFR_MEM8 here and provide it a storage
 // location so that the test code can read/write on it.
-extern unsigned char sfr_store;
-#define _SFR_MEM8(mem_addr) sfr_store
+// While at it, also cover the 16 and 32 bit macros.
+
+extern uint8_t  sfr8_store;
+extern uint16_t sfr16_store;
+extern uint32_t sfr32_store;
+
+#define _SFR_MEM8(mem_addr)  sfr8_store
+#define _SFR_MEM16(mem_addr) sfr16_store
+#define _SFR_MEM32(mem_addr) sfr32_store
+
+// Notice that the above will map every single 8 bit register defined
+// with _SFR_MEM8 into the same memory location. For some testing that is
+// perfectly fine, but if you for instance are testing that a function
+// updates a register a certain way, that has a high chance of breaking
+// when more than one register is being used.
+//
+// Long term it would be better to provide a separate storage for
+// each register. In the real avr header files, _SFR_MEM8(mem_addr)
+// is wrapping _MMIO_BYTE(mem_addr) which expands to volatile memory
+// access of the memory address. The ADCSRA register might be defined as
+// _SFR_MEM8(0x7A). Other registers use higher address values like 0x80B,
+// so there is too much variation and too high values to use the memory
+// address as an index in an array.
+//
+// In theory it would be possible to use macro token concatenation and
+// generate unique variables like sfr_store_0X7A but it would fail for
+// hex char casing differences (0x7a vs 0X7A) and would either require
+// pre-declaring a huge amount variables for all the possible addresses
+// that could be used, or require some tool to parse the relevant header
+// files and generate code.
+//
+// In my opinion the most viable option to support distinct storage for
+// each register would be to lazily create register storages as they are
+// requested and store them in a linked list with the address as a key. So
+// the _SFR_MEM8 macro would then wrap to a function ala
+//
+//     find_or_create_storage(mem_addr) {
+//         node = find_in_list(mem_addr)
+//         if (!node) {
+//             node = create_node(mem_addr)
+//             add_to_list(node)
+//         }
+//         return node
+//     }
+

In addition I have a couple of updates to the avr/interrupt.h file (cli macros etc). I can push some branches with these changes to merge/cherry-pick from.

[PRosenb]

An other option would be to create a new directory that is added to the include path.

e.g. cpp/avr_includes/avr/sleep.h
And then add cpp/avr_includes to the include path.

[ianfixes]

@hlovdal what I'm hearing re: #include <avr/io.h> is that I am missing some unit tests for arduino_ci itself -- I need to verify that libraries that include any of <avr/*.h> will still compile when in use by the unit test wrapper.

@PRosenb I'm going to start a separate avr branch of this repo where we can collaborate on this and then merge all these things into master. The improvements that you've both made are better than what I could come up with, as I lack the experience in this area of Arduino functionality.

The shortlist of what will be ironed out in the avr branch:

• AvrRegisters.{h,cpp}
• combined avr/interrupt.h work
• cpp/avr_includes/avr or maybe just cpp/avr, depending on how things are interlinked
• _SFR_MEM8

I will create issues for these momentarily, with an avr label.

[ianfixes]

@hlovdal this discussion has picked back up in #187 and #183 if you are interested in the direction this is taking