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.

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
merged 14 commits into from
Nov 19, 2020
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
78 changes: 78 additions & 0 deletions SampleProjects/TestSomething/test/wire.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
#include <ArduinoUnitTests.h>
#include <Arduino.h>
#include <Wire.h>
using std::deque;

unittest(begin_write_end) {
// master write buffer should be empty
deque<uint8_t>* mosi = Wire.getMosi(14);
assertEqual(0, mosi->size());

// write some random values to random slave
Wire.begin();
Wire.beginTransmission(14);
Wire.write(0x07);
Copy link
Collaborator

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

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

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();

// check master write buffer values
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;
// place some values on random slaves' read buffers
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);

// check read buffers and read-related functions
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());

// write some values to different random slave
Wire.beginTransmission(47);
for (int i = 1; i < 4; i++) {
Wire.write(i * 2);
}
Wire.endTransmission();

// check master write buffer
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()
245 changes: 166 additions & 79 deletions cpp/arduino/Wire.h
Original file line number Diff line number Diff line change
@@ -1,138 +1,225 @@
/*
* The Wire Library (https://www.arduino.cc/en/Reference/Wire)
* allows you to communicate with I2C/TWI devices. The general
* TWI protocol supports one "master" device and many "slave"
* devices that share the same two wires (SDA and SCL for data
* and clock respectively).
*
* You initialize the library by calling begin() as a master or
* begin(myAddress) as a slave (with an int from 8-127). In the
* initial mock implementation we support only the master role.
*
* To send bytes from a master to a slave, start with
* beginTransmission(slaveAddress), then use write(byte) to
* enqueue data, and finish with endTransmission().
*
* When a master wants to read, it starts with a call to
* requestFrom(slaveAddress, quantity) which blocks until the
* request finishes. The return value is either 0 (if the slave
* does not respond) or the number of bytes requested (which
* might be more than the number sent since reading is simply
* looking at a pin value at each clock tick).
*
* A master can write to or read from two or more slaves in
* quick succession (say, during one loop() function), so our
* mock needs to support preloading data to be read from multiple
* slaves and archive data sent to multiple slaves.
*
* In the mock, this is handled by having an array of wireData_t
* structures, each of which contains a deque for input and a
* deque for output. You can preload data to be read and you can
* look at a log of data that has been written.
*/

#pragma once

#include <inttypes.h>
#include "Stream.h"
#include <cassert>
#include <deque>
using std::deque;

const size_t SLAVE_COUNT = 128;
const size_t BUFFER_LENGTH = 32;

struct wireData_t {
uint8_t misoSize; // bytes remaining for this read
uint8_t mosiSize; // bytes included in this write
deque<uint8_t> misoBuffer; // master in, slave out
deque<uint8_t> mosiBuffer; // master out, slave in
};

// Some inspiration taken from
// https://github.com/arduino/ArduinoCore-megaavr/blob/d2a81093ba66d22dbda14c30d146c231c5910734/libraries/Wire/src/Wire.cpp
class TwoWire : public ObservableDataStream {
private:
bool _didBegin = false;
wireData_t *in = nullptr; // pointer to current slave for writing
wireData_t *out = nullptr; // pointer to current slave for reading
wireData_t slaves[SLAVE_COUNT];

class TwoWire : public ObservableDataStream
{
public:
// constructor initializes internal data
TwoWire() {
for (int i = 0; i < SLAVE_COUNT; ++i) {
slaves[i].misoSize = 0;
slaves[i].mosiSize = 0;
}
}

// https://www.arduino.cc/en/Reference/WireBegin
// Initiate the Wire library and join the I2C bus as a master or slave. This should normally be called only once.
void begin() {
isMaster = true;
}
void begin(int address) {
i2cAddress = address;
isMaster = false;
}
// Initiate the Wire library and join the I2C bus as a master or slave. This
// should normally be called only once.
void begin() { begin(0); }
void begin(uint8_t address) {
begin((int)address);
}
void end() {
// TODO: implement
assert(address == 0);
_didBegin = true;
}
void begin(int address) { begin((uint8_t)address); }
// NOTE: end() is not part of the published API so we ignore it
void end() {}

// https://www.arduino.cc/en/Reference/WireSetClock
// This function modifies the clock frequency for I2C communication. I2C slave devices have no minimum working
// clock frequency, however 100KHz is usually the baseline.
void setClock(uint32_t) {
// TODO: implement?
}
// This function modifies the clock frequency for I2C communication. I2C slave
// devices have no minimum working clock frequency, however 100KHz is usually
// the baseline.
// Since the mock does not actually write pins we ignore this.
void setClock(uint32_t clock) {}

// https://www.arduino.cc/en/Reference/WireBeginTransmission
// Begin a transmission to the I2C slave device with the given address. Subsequently, queue bytes for
// transmission with the write() function and transmit them by calling endTransmission().
void beginTransmission(int address) {
// TODO: implement
}
// Begin a transmission to the I2C slave device with the given address.
// Subsequently, queue bytes for transmission with the write() function and
// transmit them by calling endTransmission().
// For the mock we update our output to the proper destination.
void beginTransmission(uint8_t address) {
beginTransmission((int)address);
assert(_didBegin);
assert(address > 0 && address < SLAVE_COUNT);
assert(out == nullptr);
out = &slaves[address];
out->mosiSize = 0;
}
void beginTransmission(int address) { beginTransmission((uint8_t)address); }

// https://www.arduino.cc/en/Reference/WireEndTransmission
// Ends a transmission to a slave device that was begun by beginTransmission() and transmits the bytes that were
// queued by write().
uint8_t endTransmission(uint8_t sendStop) {
// TODO: implement
// Ends a transmission to a slave device that was begun by beginTransmission()
// and transmits the bytes that were queued by write().
// In the mock we just leave the bytes there in the buffer
// to be read by the testing API and we ignore the sendStop.
uint8_t endTransmission(bool sendStop) {
assert(_didBegin);
assert(out);
out = nullptr;
return 0; // success
}
uint8_t endTransmission(void) {
return endTransmission((uint8_t)true);
}
uint8_t endTransmission(void) { return endTransmission(true); }

// https://www.arduino.cc/en/Reference/WireRequestFrom
// Used by the master to request bytes from a slave device. The bytes may then be retrieved with the
// available() and read() functions.
uint8_t requestFrom(int address, int quantity, int stop) {
// TODO: implement
return 0; // number of bytes returned from the slave device
// Used by the master to request bytes from a slave device. The bytes may then
// be retrieved with the available() and read() functions.
uint8_t requestFrom(uint8_t address, size_t quantity, bool stop) {
assert(_didBegin);
assert(address > 0 && address < SLAVE_COUNT);
assert(quantity <= BUFFER_LENGTH);
in = &slaves[address];
// do we have enough data in the input buffer
if (quantity <= (in->misoBuffer).size()) { // enough data
in->misoSize = quantity;
return quantity;
} else { // not enough data
in->misoSize = 0;
in = nullptr;
return 0;
}
}
uint8_t requestFrom(int address, int quantity) {
int stop = true;
return requestFrom(address, quantity, stop);
}
uint8_t requestFrom(uint8_t address, uint8_t quantity) {
return requestFrom((int)address, (int)quantity);
return requestFrom((uint8_t)address, (size_t)quantity, true);
}
uint8_t requestFrom(uint8_t address, uint8_t quantity, uint8_t stop) {
return requestFrom((int)address, (int)quantity, (int)stop);
}
uint8_t requestFrom(uint8_t, uint8_t, uint32_t, uint8_t, uint8_t) {
// TODO: implement
return 0;
uint8_t requestFrom(int address, int quantity, int stop) {
return requestFrom((uint8_t)address, (size_t)quantity, (bool)stop);
}

// https://www.arduino.cc/en/Reference/WireWrite
// Writes data from a slave device in response to a request from a master, or queues bytes for transmission from a
// master to slave device (in-between calls to beginTransmission() and endTransmission()).
// Writes data from a slave device in response to a request from a master, or
// queues bytes for transmission from a master to slave device (in-between
// calls to beginTransmission() and endTransmission()).
size_t write(uint8_t value) {
// TODO: implement
return 0; // number of bytes written
assert(out);
assert(++(out->mosiSize) <= BUFFER_LENGTH);
(out->mosiBuffer).push_back(value);
return 1; // number of bytes written
}
size_t write(const char *str) {
return str == NULL ? 0 : write((const uint8_t *)str, String(str).length());
}
size_t write(const char *str) { return str == NULL ? 0 : write((const uint8_t *)str, String(str).length()); }
size_t write(const uint8_t *buffer, size_t size) {
size_t n;
for (n = 0; size && write(*buffer++) && ++n; --size);
for (n = 0; size && write(*buffer++) && ++n; --size)
;
return n;
}
size_t write(const char *buffer, size_t size) { return write((const uint8_t *)buffer, size); }
size_t write(const char *buffer, size_t size) {
return write((const uint8_t *)buffer, size);
}
size_t write(unsigned long n) { return write((uint8_t)n); }
size_t write(long n) { return write((uint8_t)n); }
size_t write(unsigned int n) { return write((uint8_t)n); }
size_t write(int n) { return write((uint8_t)n); }

// https://www.arduino.cc/en/Reference/WireAvailable
// Returns the number of bytes available for retrieval with read(). This should be called on a master device after a
// call to requestFrom() or on a slave inside the onReceive() handler.
// Returns the number of bytes available for retrieval with read(). This
// should be called on a master device after a call to requestFrom() or on a
// slave inside the onReceive() handler.
int available(void) {
// TODO: implement
return 0; // number of bytes available for reading
assert(in);
return in->misoSize;
}

// https://www.arduino.cc/en/Reference/WireRead
// Reads a byte that was transmitted from a slave device to a master after a call to requestFrom() or was transmitted
// from a master to a slave. read() inherits from the Stream utility class.
int read(void) {
// TODO: implement
return '\0'; // The next byte received
// Reads a byte that was transmitted from a slave device to a master after a
// call to requestFrom() or was transmitted from a master to a slave. read()
// inherits from the Stream utility class.
// In the mock we simply return the next byte from the input buffer.
uint8_t read(void) {
uint8_t value = peek();
--in->misoSize;
in->misoBuffer.pop_front();
return value; // The next byte received
}
int peek(void) {
// TODO: implement
return 0;

// part of the Stream API
uint8_t peek(void) {
assert(in);
assert(0 < in->misoSize);
return in->misoBuffer.front(); // The next byte received
}

// part of the Stream API
void flush(void) {
// TODO: implement
// NOTE: commented out in the megaavr repository
// data already at the (mock) destination
}

// https://www.arduino.cc/en/Reference/WireOnReceive
// Registers a function to be called when a slave device receives a transmission from a master.
void onReceive( void (*callback)(int) ) {
// TODO: implement
}
// Registers a function to be called when a slave device receives a
// transmission from a master.
// We don't (yet) support the slave role in the mock
void onReceive(void (*callback)(int)) { assert(false); }

// https://www.arduino.cc/en/Reference/WireOnRequest
// Register a function to be called when a master requests data from this slave device.
void onRequest( void (*callback)(void) ) {
// TODO: implement
}
// Register a function to be called when a master requests data from this
// slave device.
// We don't (yet) support the slave role in the mock
void onRequest(void (*callback)(void)) { assert(false); }

private:
int i2cAddress;
bool isMaster = false;
// testing methods
bool didBegin() { return _didBegin; }

deque<uint8_t> *getMiso(uint8_t address) {
return &slaves[address].misoBuffer;
}
deque<uint8_t> *getMosi(uint8_t address) {
return &slaves[address].mosiBuffer;
}
};

extern TwoWire Wire;