search

MrYsLab / pymata-aio

This is the second generation PyMata client.
https://github.com/MrYsLab/pymata-aio/wiki
GNU Affero General Public License v3.0
155 stars 51 forks source link

Minor updates to pymata_core.py #76

Closed TeamWildCards closed 6 years ago

TeamWildCards commented 6 years ago

Miscellaneous fixes, cleanup, and enhancements to pymata_core.py: -Resolved issue #74 -Convert firmware name from two 7-bit bytes into characters upon receipt -Changed a variable name from "pin" to "port" to in _command_dispatcher -Changed I2C_RESTART_TX to I2C_END_TX_MASK in comment/description of i2c_read_request

MrYsLab commented 6 years ago

I am not clear if we are in agreement about I2C_END_TX_MASK or not. Do you agree that my original code for that section is correct?

Everything else looks good so I will merge once we agree on I2C_END_TX_MASK.

TeamWildCards commented 6 years ago

The functional code itself is correct, but the supporting comments are incorrect when they suggest OR'ing with I2C_RESTART_TX as needed. In constants.py, I2C_RESTART_TX = 0. Technically, I2C_RESTART_TX is only used privately/internally in Firmata and does not need to be defined in PyMata. If someone were to perform an OR with I2C_RESTART_TX, there would be no effect because all of its bits are zeros.

Instead, the comment should suggest OR'ing with I2C_END_TX_MASK as needed. Constants.py defines it as a bitmask: I2C_END_TX_MASK = 0B01000000.

Again, the functional code is correct, there is just incorrect guidance in the comments.

On Mon, Aug 20, 2018, 5:04 PM Alan Yorinks notifications@github.com wrote:

I am not clear if we are in agreement about I2C_END_TX_MASK or not. Do you agree that my original code for that section is correct?

Everything else looks good so I will merge once we agree on I2C_END_TX_MASK.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/MrYsLab/pymata-aio/pull/76#issuecomment-414478383, or mute the thread https://github.com/notifications/unsubscribe-auth/AiZCvEStNKmj0oJgAHOwFnvzopnYZ46zks5uSzJQgaJpZM4WDSCz .

MrYsLab commented 6 years ago

You are correct. Merge is done.