search

UbiquityRobotics / ubiquity_motor

Package that provides a ROS interface for the motors in UbiquityRobotics robots
BSD 3-Clause "New" or "Revised" License
25 stars 24 forks source link

I2C Lock #104

Closed dorkamotorka closed 4 years ago

dorkamotorka commented 4 years ago

Merged the write() and read() calls. Changed the chipRegAddr argument to the function, so that it represents the i2c slave's register from which we want to read.

rohbotics commented 4 years ago

Mark, this already buys us a bunch even without the oled node doing it. It's not a lock but an actual kernel level transactional interface. This means that nothing else will be allowed down the bus until this read/write cycle is complete. It's not just adding a lock that other users must make sure to grab.

I don't see much use in spinning out such a small piece of code into a library, I am ok with keeping it in each node that needs to do I2C. Because ubiquity_motor specifically doesn't need multi byte reads, I think its worth simplifying the code in here to remove that support.

Rohan

mjstn commented 4 years ago

I should have been a bit more clear. Yes this is of value and I will test and we can approve and push at that time, likely in a day or two.

I was bringing up that at some later date in a different change be used in oled module. NOT for this, for later.

The other thing I discussed is a larger project for another time, not for this change. I was sort of 'thinking out loud' about future work that could be done to form a library usable by at least 3 of our repositories, one of which does not exist yet (accelerometer/gyro). Pay no attention to that I have to sort out starting a backlog task would be best.

mjstn commented 4 years ago

There was a bug in the earlier routine that misled you to feel the write was needed. That is 'my bad'.
The comment was what we want and the code itself should have been a test to only do the write if chipRegAddr >= 0.

Here is buggy code that you were apparently simply making your code do the same thing. Again, my bad. if (chipRegAddr < 0) { // Suppress reg address if negative value was used

dorkamotorka commented 4 years ago

So now there is only a read call (probably I could make a bit more informative) - the same as what the last version of Mark's code should do (without the above mentioned bug). I think it is up to the testing to show whether this works.

If the code doesn't work, we should also try to change the MACRO in https://github.com/UbiquityRobotics/ubiquity_motor/blob/b7edbdbcec08d89b3702b68b3a3bbf27a6e51dab/src/motor_hardware.cc#L877 from O_RDWR (open for writing and reading) to O_RDONLY (open for reading only).

I do have a question.. Is our hardware compatible with SMBus protocol or is it I2C only?

mjstn commented 4 years ago

The part that is being discussed, PCF8574, is a very simple minded pure I2C device. I will update for this branch and test the 'read only' mode. One thing that is interesting in the old bug is that the reason it worked is a -1 was sent which is hex 0xFF so in fact the write that should not have been done was ok in that it told the part all bits were inputs (1) Funny in that way.