Update I2C_READ_CONTINUOUSLY to match I2C_READ w/ no provided slaveRegister #155
Conversation
| 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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Got it.
Edit:
Should this have shown up when I was testing the patch?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, we never really planned for unit tests when writing Firmata so some things may not be testable.
There was a problem hiding this comment.
Thanks for the tips
rwaldron
force-pushed
the
i2c-read-continuous-without-slave
branch
from
7a25bc5 to
cea58a2
Compare
|
Updated |
|
@rwaldron I'm going to merge this pull request (but only this one, not the one against configurable-dev just yet). I plan to cut another 2.4 beta release this weekend (likely later today). I would really like to get this next beta into circulation to help find any issues. I'll let you know when it's available and would love if you could get J5 community using it for a few days. |
Update I2C_READ_CONTINUOUSLY to match I2C_READ w/ no provided slaveRegister
|
Sounds good, I haven't had a chance to start on the unit tests, hopefully this week. |
|
Ping me when the beta is ready |
|
Nevermind, just saw the other email ;) |
No description provided.