High-level wire implementation, complete with unit tests

[lsaca05]

Fixes #181

Tracks addresses read from/written to on a high level using maps and vectors, as well as the content at that address, of course.

Let me know if you'd like more tests, though I think I covered everything that we deemed useful at the moment.

[lsaca05]

I'm guessing the test failures are related to me overwriting the map from AvrMath with the C++ class template (lines 25-27). I'm assuming there isn't as shorthand a way to fix it like the max function in lines 7-22.

[jgfoster]

Rather than removing functionality that is part of the core Arduino system (map), we should use the C++ namespace feature to avoid the name conflict. So, remove line 36 and replace all your references to map with std::map. Then remove lines 24-29.

[lsaca05]

Rather than removing functionality that is part of the core Arduino system (map), we should use the C++ namespace feature to avoid the name conflict. So, remove line 36 and replace all your references to map with std::map. Then remove lines 24-29.

I tried this and my terminal gets flooded with errors related to the Arduino core map being used incorrectly. I deemed that ifdef patch job necessary at first, but I'll try and play around with namespaces to confirm when I get a chance.

[jgfoster]

Ah, yes, I see. The existing macro will indiscriminately changes to all your uses of map and that's bad. And, you are right, the best solution would be something like what was done with max(). Absent that, we may need to come up with something else. This requires more thought.

[jgfoster]

Let's try to rewrite this to use something other than a map.

[jgfoster]

@ianfixes, With a couple minor edits, this is an implementation of Wire that I'd like to have you consider. Please let me know your thoughts. Thanks!

[jgfoster]

@lsaca05, Looks good. One more thing (for now!). Could you edit the initial description so that instead of "See #181" it says "Fixes #181"? That will automatically "link" the issue to the PR and close the issue when/if the PR is merged. Thanks!

[ianfixes]

There are a lot of random-looking numbers in this unit test, which might be better expressed as named variables. Otherwise, please add some comments that explain the different sections of the tests

[jgfoster]

Rather than named variables, let's just add comments along the line of "try some random value (0x07) sent to some random slave (14)."

[ianfixes]

With the aid of your comments I can see what you're after. But I'm going to insist on using named constants.

The reason for this is purely readability; as a maintainer I have to scan a lot of contributions in a short amount of time and solve problems in them. Consider the following chunk of this test as currently written:

    assertEqual(1, Wire.read());
    assertEqual(2, Wire.available());
    assertEqual(4, Wire.read());
    assertEqual(1, Wire.available());
    assertEqual(7, Wire.read());

This requires me (and any future contributors to this code) to puzzle out things like

• the 1, 4, and 7, here being conceptually related to each other (and to a definition a dozen lines earlier) even though the values no longer appear on adjacent lines
• the 1 in assertEqual(1, Wire.read()); and assertEqual(1, Wire.available()); represent completely different things even though they are are expressed in the exact same way

This affects numeric tests more than strings or objects (which end up being a bit more self-descriptive).

Here's an example of how I'd like this to be written:

    const uint8_t randomSlave = 14;
    const uint8_t randomValues[] = { 0x07, 0x0E };
    Wire.begin();
    Wire.beginTransmission(randomSlave);
    Wire.write(randomValues[0]);
    Wire.write(randomValues[1]);
    Wire.endTransmission();

    // check master write buffer values
    assertEqual(2, mosi->size());
    assertEqual(randomValues[0], mosi->front());
    mosi->pop_front();
    assertEqual(randomValues[1], mosi->front());
    mosi->pop_front();
    assertEqual(0, mosi->size());

Similar for the values 19 and 34 -- these should be named variables, so that the function calls can be more indicative of what is being input.

    Wire.begin();
    deque<uint8_t>* miso;

    // place some values on random slaves' read buffers
    const int aRandomSlave = 19;
    const int anotherRandomSlave = 34;
    const uint8_t moreRandomValues = { 1, 4, 7 };
    miso = Wire.getMiso(aRandomSlave);
    miso->push_back(randomValues[0]);
    miso->push_back(randomValues[1]);
    miso = Wire.getMiso(anotherRandomSlave);
    miso->push_back(moreRandomValues[0]);
    miso->push_back(moreRandomValues[1]);
    miso->push_back(moreRandomValues[2]);

I chose random in the naming to reflect your comment, but expected (e.g. expectedIntValues / expectedByteValues) or input also work. Or for things where you just have to pick a wire or port to work with, const int arbitraryIndex = 19, etc. What needs to be conveyed is the difference between a specific value (e.g. a size) that's expected to be returned and an arbitrary number that just needs to match the input.

[ianfixes]

I appreciate the work that has gone into this, especially the unit tests that cover the added functionality. @jgfoster let me know when your comments are addressed

[jgfoster]

@ianfixes, if you are satisfied with the comments in the test file, then I'm happy to have it merged. Thanks!

[ianfixes]

Thanks for taking the time to add clarity to those test cases!

I think this is just missing the CHANGELOG.md entry, but I can add that if you prefer