Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ianfixes merged 14 commits into from
Nov 19, 2020
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
bb61617
Added crude high level Wire implementation + Wire unit tests.
lsaca05 Oct 19, 2020
4989505
Forgot to add new wire test file
lsaca05 Oct 19, 2020
13862bc
me dumb and fixed wire tests, though there's an anomaly in compilation
lsaca05 Oct 20, 2020
226bc18
Completed wire implementation with address tracking via maps + tests
lsaca05 Oct 26, 2020
42aaa39
Wire implementation converted from map to deque, courtesy of James Fo…
lsaca05 Nov 3, 2020
6ddd254
Here's a newline for you:
lsaca05 Nov 3, 2020
194af8b
Merge branch 'master' of https://github.com/Arduino-CI/arduino_ci int…
lsaca05 Nov 8, 2020
d6c3bb8
Cleaned up crude fixes for name clashes in wire header.
lsaca05 Nov 8, 2020
2821146
Removed requestFrom() overload to reduce ambiguity. Added wire test c…
lsaca05 Nov 10, 2020
01eeaf3
Forgot to add test file to last commit. This is the real commit addin…
lsaca05 Nov 10, 2020
7593ab8
Updated wire tests to be more readable. Added additional assertion fo…
lsaca05 Nov 18, 2020
f2fd95a
Forgot to scroll down and update half of the last test for last commi…
lsaca05 Nov 18, 2020
bb25c28
Merge branch 'master' of https://github.com/Arduino-CI/arduino_ci int…
lsaca05 Nov 18, 2020
e4fd2e4
Removed the assertion added 3 commits ago. Didn't realize failed requ…
lsaca05 Nov 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions SampleProjects/TestSomething/test/wire.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
#include <ArduinoUnitTests.h>
#include <Arduino.h>
#include <Wire.h>
using namespace std;

unittest(begin_write_end) {
deque<uint8_t>* mosi = Wire.getMosi(14);
assertEqual(0, mosi->size());
Wire.begin();
Wire.beginTransmission(14);
Wire.write(0x07);
Copy link
Collaborator

@ianfixes ianfixes Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jgfoster jgfoster Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@ianfixes ianfixes Nov 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Wire.write(0x0E);
Wire.endTransmission();
assertEqual(2, mosi->size());
assertEqual(0x07, mosi->front());
mosi->pop_front();
assertEqual(0x0E, mosi->front());
mosi->pop_front();
assertEqual(0, mosi->size());
}

unittest(readTwo_writeOne) {
Wire.begin();
deque<uint8_t>* miso;
miso = Wire.getMiso(19);
miso->push_back(0x07);
miso->push_back(0x0E);
miso = Wire.getMiso(34);
miso->push_back(1);
miso->push_back(4);
miso->push_back(7);

assertEqual(0, Wire.requestFrom(19, 3));
assertEqual(2, Wire.requestFrom(19, 2));
assertEqual(2, Wire.available());
assertEqual(0x07, Wire.read());
assertEqual(1, Wire.available());
assertEqual(0x0E, Wire.read());
assertEqual(0, Wire.available());
assertEqual(3, Wire.requestFrom(34, 3));
assertEqual(3, Wire.available());
assertEqual(1, Wire.read());
assertEqual(2, Wire.available());
assertEqual(4, Wire.read());
assertEqual(1, Wire.available());
assertEqual(7, Wire.read());
assertEqual(0, Wire.available());

Wire.beginTransmission(47);
for (int i = 1; i < 4; i++) {
Wire.write(i * 2);
}
Wire.endTransmission();
deque<uint8_t>* mosi = Wire.getMosi(47);

assertEqual(3, mosi->size());
assertEqual(2, mosi->front());
mosi->pop_front();
assertEqual(2, mosi->size());
assertEqual(4, mosi->front());
mosi->pop_front();
assertEqual(1, mosi->size());
assertEqual(6, mosi->front());
mosi->pop_front();
assertEqual(0, mosi->size());
}

unittest_main()
Loading