Re: [PATCH v2] hw/sensor: Add lsm303dlhc magnetometer device

[Peter Maydell]

On Mon, 20 Sept 2021 at 14:38, Kevin Townsend <kevin.townsend@linaro.org> wrote:
>
> Hi Peter,
>
> Thanks for the review, and sorry for the slow reply, just getting back from 
> holidays myself.
>
> On Thu, 26 Aug 2021 at 17:39, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>>
>> So if I'm reading the datasheet correctly, the LM303DLHC is
>> really two completely distinct i2c devices in a single
>> package with different slave addresses; this QEMU device
>> implements only the magnetometer i2c device, and if we wanted
>> to add the accelerometer device we'd implement that as a
>> second separate QEMU i2c device ?
>
>
> This is correct. There are two distinct dies in the chip with separate I2C 
> addresses, etc.,
> and this should probably be modelled separately. I chose the magnetometer 
> since it's
> a far simpler device to model than the accelrometer, but still solves the 
> need for a
> more complex I2C sensor that can be used in testing with the I2C bus.
>
>> > +    if (value > 2047 || value < -2048) {
>> > +        error_setg(errp, "value %d lsb is out of range", value);
>>
>> Why "lsb" ?
>>
>
> In my head, using LSB seemed more precise since I know exactly what value will
> be set to the registers, and exactly what I will get back when reading versus 
> passing
> in a float that's needs to be conveted as a 'best-fit' scenario in 0.125°C 
> steps?

My question was really, "what does 'lsb' mean here"?  I would usually
assume "least significant bit", but that makes no sense in this context.


>> > +
>> > +/**
>> > + * @brief Callback handler when DeviceState 'reset' is set to true.
>> > + */
>> > +static void lsm303dlhc_mag_reset(DeviceState *dev)
>> > +{
>> > +    I2CSlave *i2c = I2C_SLAVE(dev);
>> > +    LSM303DLHC_Mag_State *s = LSM303DLHC_MAG(i2c);
>> > +
>> > +       /* Set the device into is default reset state. */
>> > +       lsm303dlhc_mag_default_cfg(&s->parent_obj);
>>
>> Misindentation.
>>
>> Also, don't use the parent_obj field;
>> always use the QOM cast macro when you need the pointer
>> to something as a different type. In this case you already
>> have the I2CSlave*, in 'i2c'. But better would be to make
>> lsm303dlhc_mag_default_cfg() take a LSM303DLHC_Mag_State*
>> directly rather than taking an I2CSlave* and casting it
>> internally.
>
>
> Do you have an example, just to be sure I follow? I've changed the code
> as follows:
>
> static void lsm303dlhc_mag_reset(DeviceState *dev)
> {
>     I2CSlave *i2c = I2C_SLAVE(dev);
>     LSM303DLHCMagState *s = LSM303DLHC_MAG(i2c);
>
>     /* Set the device into its default reset state. */
>     lsm303dlhc_mag_default_cfg(s);
> }
>
> static void lsm303dlhc_mag_default_cfg(LSM303DLHCMagState *s)
>
> Is this sufficient?

Yes, that's right.

>> > +static void lsm303dlhc_mag_initfn(Object *obj)
>> > +{
>> > +    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGX, "int",
>> > +                lsm303dlhc_mag_get_xyz,
>> > +                lsm303dlhc_mag_set_xyz, NULL, NULL);
>> > +
>> > +    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGY, "int",
>> > +                lsm303dlhc_mag_get_xyz,
>> > +                lsm303dlhc_mag_set_xyz, NULL, NULL);
>> > +
>> > +    object_property_add(obj, LSM303DLHC_MSG_PROP_MAGZ, "int",
>> > +                lsm303dlhc_mag_get_xyz,
>> > +                lsm303dlhc_mag_set_xyz, NULL, NULL);
>>
>> What units are these in? It looks like your implementation just
>> uses the property values as the raw -2048..+2048 value that the
>> X/Y/Z registers read as. Would it be better for the properties to
>> set the value in Gauss, and then the model to honour the
>> gain settings in CRB_REG_M.GN{0,1,2} ?  That way guest code that
>> adjusts the gain will get the results it is expecting.
>
>
> I guess I found raw units that the sensor uses more intuitive personally,
> with no room for unexpected translations and not having to deal with floats,
> but if you feel gauss or degrees C is better, I can change these.

Well, given that the device specifically changes the value it
shows the guest based on guest-programmable gain settings,
it does seem to me to be more natural to specify the values
in some way that represents the "real world data" that the
sensor is measuring. Ideally we would then if/when we add more
magnetometer implementations have them all use the same units,
for consistency. This is the first magnetometer we have, so this
is where we get to pick the convention.

> In that case, should I accept floating point inputs, however, or stick to 
> integers?
> When dealing with magnetometers the values can be very small, so it's not the
> same as a temp sensor where we can provide 2300 for 23.00C.

What sort of range and precision requirements are we talking about
here? If we can avoid having to use float that would be nice...

>>
>> > +
>> > +    object_property_add(obj, LSM303DLHC_MSG_PROP_TEMP, "int",
>> > +                lsm303dlhc_mag_get_temperature,
>> > +                lsm303dlhc_mag_set_temperature, NULL, NULL);
>>
>> What units is this in?
>
>
> LSB where 1 LSB = 0.125 C, documented elsewhere, but as per the above
> I can change this to degrees if you can clarify if this should be in float or 
> something
> integere based with a specific scale factor.

Our existing temperature sensors use integer properties whose
value is "temperature in degrees C, units of 0.001 C".
Consistency with that would be best. (We should write these
conventions down somewhere. Not sure where...)

thanks
-- PMM