Possible direction for __ARDUINO_CI_SFR_MOCK

[jgfoster]

This seems to hint at a direction to take for #187. Before merging please see SPI.h:100 where a change was made in order to get the spi test in godmode.cpp to pass. It is almost certainly wrong, but I don't know enough to fix it.

This is also related to discussions in #183.

[matthijskooijman]

For the SPI thing, see the related commit 90dc747 from #144.

I suspect that:

• With the old definition, any IO register would resolve to its address, rather than a meaningful value.
• For the 328p, this means that SPCR would resolve to 0x2C:

  arduino_ci/cpp/arduino/avr/iom328p.h

  Line 287 in f6ca7a7

  #define SPCR _SFR_IO8(0x2C)

• Since 1 << DORD == 0x20, the SPI library check if (!(SPCR & (1 << DORD))) { would end up as false, ending up in the else.
• With this PR, SPCR is actually being read, but since nothing writes it, this ends up reading 0, and the if ends up as true, using the wrong byte order.
• This if is in the mock SPI library to support sketches that call SPI.begin() and then manually change SPCR to use a non-standard byte order (lacking a standard Arduino API to do this). Note that with the current _SFR_IO8, those sketches could never have worked, though, since assigning SPCR breaks.

If the above is correct, I think a correct fix could be to set a meaningful value for SPCR in the SPI.begin() method. At least DORD should be set, maybe the (code to calculate the) value should just be copied from the AVR SPI library? This only applies to __AVR__, since the SPCR "API" is AVR-specific (so both writes and reads from SPCR should be guarded by __AVR__, like my commit does for the read).

[ianfixes]

If we rebase this off master (which should now have the __AVR__ guard in place) will that resolve all the concerns here?

[jgfoster]

I've taken a bit of a different approach than suggested by @matthijskooijman above. Instead of using (SPCR & (1 << DORD) to determine MSBFIRST or LSBFIRST I am saving it from the beginTransmission() call (the previous mock code was simply ignoring the parameters). This makes more sense to me but that doesn't mean that it is right!

Another area that should be considered is the long long at the end of Godmode.cpp. There was a suggestion that it be uint8_t (bytes) since this more accurately represents the actual registers. On the other hand, I notice that there are attempts to use 16-bit or 32-bit pointers and that could cause an alignment error.

But, from what I have, I'm satisfied with this PR and I think it is an improvement on the previous situation.

Advice, as always, is welcome.

[ianfixes]

I think it is an improvement on the previous situation

As do I... my round of edits on this all ended in some segfaulting unit tests. I've squashed this into #183 and we'll see where it takes us

[matthijskooijman]

Another area that should be considered is the long long at the end of Godmode.cpp. There was a suggestion that it be uint8_t (bytes) since this more accurately represents the actual registers. On the other hand, I notice that there are attempts to use 16-bit or 32-bit pointers and that could cause an alignment error.

Good point about alignment. Maybe the AVR registers happen to be aligned, but I wouldn't really expect so (and if so, probably only 16-bit, not 32-bit). I was thinking reading registers could be more explicit, e.g.:

 #define _SFR_IO16(io_addr) ((uint16_t)__ARDUINO_CI_SFR_MOCK[io_addr + 1] << 8 | __ARDUINO_CI_SFR_MOCK[io_addr]))

But that no longer allows assigning values to the register, so that won't fly...

I'm not actually sure how to handle this in a way that:

• Allows assignment
• Does not violate alignment
• Merges two adjacent _SFR_IO8 accesses in a single _SFR_IO16 access.
• Does not violate strict aliasing rules (can probably be done using a union)
• Works in C (in C++, you can probably pull this off with custom assignment operators)

Mostly alignment is a problem, anyone know what the rules are for that? IIRC on some platforms misaligned access is a fatal problem, on a lot of others it's just slower. Can you somehow tell the compiler that you're not sure a pointer is aligned so it can generate slower but safe byte-by-byte accesses automatically maybe?

[jgfoster]

It appears that x86 allows misaligned memory accesses (see this stackoverflow question), so I think that changing this to a byte would be safe and accurate. I'll update this PR.

[jgfoster]

@matthijskooijman, This time I've taken the approach you suggested above (I appreciate your patience and explanations). Further review and comments are welcome.

@ianfixes, I've added tests for bit/byte order and for misaligned memory accesses. I think this a further improvement and invite you to merge it with the previous code.

[ianfixes]

I think I have this all picked in again

[ianfixes]

It's failing here:
https://ci.appveyor.com/project/ianfixes/arduino-ci/builds/36287425#L3081

I'm not sure whether that's because a separate patch in SPI.h had me wrapping the bitOrder == MSBFIRST test in a preprocessor check:

    #if defined(SPCR) && defined(DORD)
    if (bitOrder == MSBFIRST) {

Is defined(SPCR) && defined(DORD) still relevant here? I'm going to try without anyway, but I'm curious if that's correct.

[jgfoster]

Yes, the failing tests are related to confirming that the two bit-order configs each work. When the code used SPCR and DORD then they needed to be guarded with a defined() check. But now that we are back to using a private instance variable to remember the bit order, there is no need to guard the bit order check.

[matthijskooijman]

Agreed, the changes from this PR make the guard obsolete. You can just drop my commit 7d7d810 completely.

[ianfixes]

A squashed version of this was merged with #183