RaemondBW / SWire

Software implementation of I2C for Arduino (Tested with the atmega328p based boards)
MIT License
3 stars 1 forks source link

A few things I noticed. #1

Open Koepel opened 7 years ago

Koepel commented 7 years ago

Hi, I have not tested your SWire library yet, but I noticed a few things.

The Wire.available() should return the number of valid bytes in the rxBuffer.

Writing a 16-bit value is not according to the Arduino Wire library. Please don't add that. It is very confusing and might result into different functionality between the Arduino Wire library and your SWire library. I will guaranteed cause a lot of trouble.

You mix HIGH and LOW with 1 and 0. They are the same, but from a programmers viewpoint they are different things. Perhaps you can make it more obvious what is used in the code.

The buffer length is only 8 bytes ? The Arduino Wire library for AVR chips has 32 bytes.

Is it possible to do a Wire.beginTransmission() followed by a Wire.endTransmission() to test the ACK of a sensor ? That is used in i2c scanner sketches. I did not test it, and I can not tell from the source if that will work.

What about making it more compatible ? Perhaps a (dummy) setClock(), a peek(), a end() and a flush(), en perhaps some functions from the Stream class.

RaemondBW commented 7 years ago

Hi, I am surprised you found this so soon after publishing... :) The documentation and code cleanliness is definitely lacking. Also I am definitely open to Pull requests :)

Writing a 16-bit value is not according to the Arduino Wire library. Please don't add that. It is very confusing and might result into different functionality between the Arduino Wire library and your SWire library. I will guaranteed cause a lot of trouble.

noted. It was useful for my single usecase and can be updated to follow the Wire Interface more closely.

You mix HIGH and LOW with 1 and 0. They are the same, but from a programmers viewpoint they are different things. Perhaps you can make it more obvious what is used in the code.

Will fix.

The buffer length is only 8 bytes ? The Arduino Wire library for AVR chips has 32 bytes.

Will fix.

Is it possible to do a Wire.beginTransmission() followed by a Wire.endTransmission() to test the ACK of a sensor ? That is used in i2c scanner sketches. I did not test it, and I can not tell from the source if that will work.

I haven't tried this particular use case yet. I can investigate.

What about making it more compatible ? Perhaps a (dummy) setClock(), a peek(), a end() and a flush(), en perhaps some functions from the Stream class.

This is all definitely possible.

Koepel commented 7 years ago

I'm surprised as well, that I found your software I2C so soon.

I noticed one more thing: Your .endTransmission() has already a 'stop' parameter, but the .requestFrom() should also get a 'stop' parameter. Is a repeated start implemented ? That's another thing to add.

Did you know that the SCL can be combined for multiple busses ? That means that a number of objects can be created and .begin(sda,scl) can be called a number of times with the same SCL pin. That reduces the number of used pins.

RaemondBW commented 7 years ago

I have made a couple of the changes and more are in a pr awaiting my time to verify on hardware.

on the mix of HIGH and LOW with 1 and 0. Do you have any suggestions? It is complicated because in some cases it doesn't make sense to use a 1 or 0. I could just make a secondary function, setOne or setZero that calls the High and Low functions.

Koepel commented 7 years ago

I was talking about digitalRead(). That function returns HIGH or LOW. Therefor readBit() and readAck() return HIGH or LOW. In readByte(), the Arduino macro bitWrite() is used, assuming LOW is 0 and HIGH is 1. In readNack() the readBit() is compared to 1, assuming HIGH is 1. It is okay of course, but when looking at it, the HIGH and LOW suddenly change into 1 and 0. I suggest to also add comment to readBit() that it returns 0 or 1. Then it is defined what it is. If you are a purist, you could translate HIGH and LOW into 1 or 0 in readBit() and readAck().

RaemondBW commented 7 years ago

Just checked i2c scanning and confirmed it works.

Please try the library and let me know if it works for you. I believe I updated most of the existing API to much closer follow the Arduino library.

Koepel commented 7 years ago

Test with Arduino Uno and HMC5883L with level shifter.