Fix I2C address

[jerabaul29]

Summary

I bumped my head trying to use the ISM330DHCX over I2C with this library.

Upon debugging, it seems like the issue was with the address used in the I2C wrappers - I dont know why there is some extra logics i.e.:

((uint8_t)(((address) >> 1) & 0x7F))

instead of just

address

which would be what I would expect: the first argument of these I2C functions should just be the I2C address of the slave device.

Validation

Things now work using the ISM330DHCX over I2C.

[cparata]

Hello @jerabaul29 ,
the I2C address used in the library is in the 8-bit format (0xD5 when the SA0 pin is low and 0xD7 when the SA0 pin is high) like explained in the Table 10 of the Datasheet. Instead, the STM32duino Core manages the I2C address in the 7-bit format and this is the reason why you find that formula inside the driver implementation. All MEMS libraries in the STM32duino repository uses the same rule, so I have to reject this PR.
Finally, you already have the defines for the I2C addresses in the ism330dhcx_reg.h file; so, you just need to adapt the constructor according your use case:

ISM330DHCXSensor(TwoWire *i2c, ISM330DHCX_I2C_ADD_H); // If SA0 pin is High

ISM330DHCXSensor(TwoWire *i2c, ISM330DHCX_I2C_ADD_L); // If SA0 pin is Low

Best Regards,
Carlo

[jerabaul29]

Thank you for the clarification! This makes sense now. So I suppose this is what this excerpt from the datasheet means? :)

"
The transaction on the bus is started through a START (ST) signal. A START condition is defined as a HIGH to
LOW transition on the data line while the SCL line is held HIGH. After this has been transmitted by the master,
the bus is considered busy. The next byte of data transmitted after the start condition contains the address of the
slave in the first 7 bits and the eighth bit tells whether the master is receiving data from the slave or transmitting
data to the slave. When an address is sent, each device in the system compares the first seven bits after a start
condition with its address. If they match, the device considers itself addressed by the master.
The Slave ADdress (SAD) associated to the ISM330DHCX is 110101xb. The SDO/SA0 pin can be used to modify
the less significant bit of the device address. If the SDO/SA0 pin is connected to the supply voltage, LSb is ‘1’
(address 1101011b); else if the SDO/SA0 pin is connected to ground, the LSb value is ‘0’ (address 1101010b).
This solution permits to connect and address two different inertial modules to the same I²C bus.
"

I see what you mean - I should just change the 7-bit address and let the logics propagate.

Thank you for the clarification. As a side note comments:

• this is a bit different from the conventions I am used to seeing in arduino codes / libraries. Of course, it is perfectly fine that you choose something else, and I agree that your way of doing likely is better. But this was a bit confusing.
• I wonder if / how there could be a warning about this somewhere, that is very easy to understand and does not require reading finely the datasheet? Once you have explained to me the logics behind your implementation I am able to find the section in the datasheet, but before that it was quite hard :) .

Not criticizing or disagreeing with your decision - just trying to make sure nobody else gets "bitten" by this :) (I spent a bit more than a full afternoon on this ^^ ).

As a side note (but this starts to diverge out a bit): when I was having the wrong address, I did not get any error code / message - just "valid" return values, + empty / all 0s stuff. I suppose this is because the Init only does stuff if using SPI:

ISM330DHCX/src/ISM330DHCXSensor.cpp

Lines 89 to 106 in 6004a94

	/**
	* @brief Configure the sensor in order to be used
	* @retval 0 in case of success, an error code otherwise
	*/
	ISM330DHCXStatusTypeDef ISM330DHCXSensor::begin()
	{
	if (dev_spi) {
	// Configure CS pin
	pinMode(cs_pin, OUTPUT);
	digitalWrite(cs_pin, HIGH);
	}
	if (Init() != ISM330DHCX_OK) {
	return ISM330DHCX_ERROR;
	}
	return ISM330DHCX_OK;
	}

and the constructor itself only sets variables.

Would there be a way to / what would be the recommended way to check for good communication with the sensor, and get an error message if using the wrong port / address etc? :)

[cparata]

Hello @jerabaul29 ,
I understood your point. It's strange that the "begin" does not return an error when you use a wrong bus address because I expect that, when the I2C address is wrong, the I2C read/write return an error. Anyway, to improve the robustness, we could add at the beginning of the "Init" function a check using the "ReadID" function and see if the return value is equal to "ISM330DHCX_ID". In this way we can check if we are able to read correctly the registers and we can return soon an error. Please, feel free to add this check in the "Init" function and submit a PR for that. Thanks in advance for your support!
Best Regards,
Carlo

[jerabaul29]

Implemented; I can close this PR for now then, and we can look at the other PR together :) .

About this address thing: maybe I can add a small section to the doc in a separate PR? I can open it and then you decide if you want to include it or not :) .