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.