Skip to content

Update I2C_READ_CONTINUOUSLY to match I2C_READ w/ no provided slaveRegister #155

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
merged 1 commit into from
Nov 2, 2014
Merged

Update I2C_READ_CONTINUOUSLY to match I2C_READ w/ no provided slaveRegister #155

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 18 additions & 8 deletions examples/StandardFirmata/StandardFirmata.ino
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ unsigned int samplingInterval = 19; // how often to run the main loop (
/* i2c data */
struct i2c_device_info {
byte addr;
byte reg;
int reg;
byte bytes;
};

Expand Down Expand Up @@ -375,10 +375,10 @@ void sysexCallback(byte command, byte argc, byte *argv)
{
byte mode;
byte slaveAddress;
byte slaveRegister;
byte data;
unsigned int delayTime;

int slaveRegister;
unsigned int delayTime;

switch(command) {
case I2C_REQUEST:
mode = argv[1] & I2C_READ_WRITE_MODE_MASK;
Expand Down Expand Up @@ -409,24 +409,34 @@ void sysexCallback(byte command, byte argc, byte *argv)
// a slave register is specified
slaveRegister = argv[2] + (argv[3] << 7);
data = argv[4] + (argv[5] << 7); // bytes to read
readAndReportData(slaveAddress, (int)slaveRegister, data);
}
else {
// a slave register is NOT specified
slaveRegister = REGISTER_NOT_SPECIFIED;
data = argv[2] + (argv[3] << 7); // bytes to read
readAndReportData(slaveAddress, (int)REGISTER_NOT_SPECIFIED, data);
}
readAndReportData(slaveAddress, (int)slaveRegister, data);
break;
case I2C_READ_CONTINUOUSLY:
if ((queryIndex + 1) >= MAX_QUERIES) {
// too many queries, just ignore
Firmata.sendString("too many queries");
break;
}
if (argc == 6) {
// a slave register is specified
slaveRegister = argv[2] + (argv[3] << 7);
data = argv[4] + (argv[5] << 7); // bytes to read
}
else {
// a slave register is NOT specified
slaveRegister = (int)REGISTER_NOT_SPECIFIED;
data = argv[2] + (argv[3] << 7); // bytes to read
}
queryIndex++;
query[queryIndex].addr = slaveAddress;
query[queryIndex].reg = argv[2] + (argv[3] << 7);
query[queryIndex].bytes = argv[4] + (argv[5] << 7);
query[queryIndex].reg = slaveRegister;
Copy link
Member

Choose a reason for hiding this comment

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

reg has a type byte which only accepts values 0-255, however REGISTER_NOT_SPECIFIED is an int with a value of -1. The type of reg in the struct should be changed from byte to int for this to work. slaveRegister should also be changed from byte to int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

Edit:

Should this have shown up when I was testing the patch?

Copy link
Member

Choose a reason for hiding this comment

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

By "testing" did you run the unit tests? I don't think there is a unit test for this yet. Or did you mean by running the code and you got the expected results? I would think without the int type that a byte set to -1 would be either 0 or 255 (I forget exactly since I hardly ever write c code anymore).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running the code for both a continuous read and non-continuous read. Either way, I made the updates.

I don't think there is a unit test for this yet.

I feel silly for not even trying :(

I will work on tests and ping when ready.

Copy link
Member

Choose a reason for hiding this comment

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

If you're feeling ambitious, a good approach would be to start a test file for I2C. You can use the encoder_test file as an example. Testing like this is really only possible with code in the configurable branch since it's modular. For now, the only test to add is to ensure that the values in the query struct (query[i].addr, query[i].reg, query[i].bytes) match the expected values when read continuously is set with and without a slave address.

I'll try to find some time to add test files for other Firmata feature classes as well.

Copy link
Member

Choose a reason for hiding this comment

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

Also, we never really planned for unit tests when writing Firmata so some things may not be testable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tips

query[queryIndex].bytes = data;
break;
case I2C_STOP_READING:
byte queryIndexToSkip;
Expand Down