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.mayd...@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?
> + * The auto-increment logic is only taken into account in this driver
> > + * for the LSM303DLHC_MAG_REG_OUT_X_H and
> LSM303DLHC_MAG_REG_TEMP_OUT_H
> > + * registers, which are the two common uses cases for it. Accessing
> either
> > + * of these registers will also populate the rest of the related
> dataset.
>
> How hard would it be to implement the behaviour correctly for all cases?
> I find it's usually better to model something correctly from the start:
> usually the person writing the code knows the h/w behaviour better than
> anybody else coming along later trying to figure out why some other
> guest code doesn't work on the model...
>
I can update this to also take auto-increment into account when you don't
start at
the first record (X-axis), yes, even if that is an uncommon scenario.
> + s->mr = 0x1; /* Operating Mode = Single conversion. */
> > + s->x = 0;
> > + s->z = 0;
> > + s->y = 0;
> > + s->sr = 0x1; /* DRDY = 1. */
>
> Do you understand how the DRDY and LOCK bits work ? The datasheet
> is unclear. Also, what's the difference between "single-conversion"
> and "continuous-conversion" modes ?
>
DRDY indicates that a set of XYZ samples is available in the data registers,
presumably post reset or when switching modes, and how LOCK works is that
once I read the first byte of the X register, the current sample will be
locked
until it has been fully read to prevent the values from being changed
mid-read
with a high sample rate. LOCK simply indicates this status of the registers
being
read-only until we get to the end of ou 6 byte sample.
Some details are unclear, however, such as what happens if I don't read
all six bytes, and go back to request the first X byte again? I'll need to
hook a
real sensor up to see what happens in those cases.
Single-conversion mode is documented in earlier variations of this chip
family, but
it is used for device calibration. From the LSM303DLH (not 'C' at the end):
"By placing the mode register into single-conversion mode (0x01), two data
acquisition cycles are made on each magnetic vector. The first acquisition
is a set pulse followed shortly by measurement data of the external field.
The second acquisition has the offset strap excited in the positive bias
mode
to create about a 0.6 gauss self-test field plus the external field. The
first
acquisition values are subtracted from the second acquisition, and the net
measurement is placed into the data output registers."
Given the lack of details in the LSM303DLHC datasheet, however, only
continuous mode should likely be modeled, and the way QEMU works
to model sensors makes this a moot point anyway since the output
registers are only updated when an end-user changes the values manually.
If specific values are expected by the data consumer, such as calibration
data
from single-conversion mode, those values can be set by the user regardless
of the current operating mode.
> > + s->ira = 0x48;
> > + s->irb = 0x34;
> > + s->irc = 0x33;
> > + s->temperature = 0; /* Default to 0 degrees C (0/8 lsb = 0 C).
> */
> > +}
> > +
> > +/**
> > + * @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?
> +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.
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.
> > +
> > + 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.
Thanks for the feedback, and sorry again for the slow reply.
-- Kevin
