Messages in this thread | | | Date | Mon, 4 Jul 2022 18:49:54 +0200 | Subject | Re: [PATCH] iio: accel: mxc4005: report orientation of accelerometer | From | Quentin Schulz <> |
| |
Hi Jonathan,
On 6/19/22 16:27, Jonathan Cameron wrote: > On Wed, 15 Jun 2022 13:02:40 +0200 > Quentin Schulz <foss+kernel@0leil.net> wrote: > >> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> >> > Hi Quentin, > > Interesting / horribly ill defined little feature ;) > >> The accelerometer can report precise values for x, y and z accelerations >> but it can also simply report its orientation on XY plane and Z axis. >> >> Since the orientation of the device may be enough information for >> userspace and allows to avoid expensive fusion algorithms, let's add >> support for it. >> >> The orientation register stores a 2b value for XY plane orientation: >> between 225° and 315°, returns 0, between 315° and 45°, 1, between 45° >> and 135°, 2 and between 135° and 225°, 3. We "round" those to 270°, >> 0°, 90° and 180° degrees. > > Wow. The datasheet description of this very confusing...
I'm relieved I'm not the only one confused by this datasheet :)
> One key thing is we need to be careful of is that tilt (x/y is > not always available - but rather shows the last, and probably > now garbage, value)
Being pedantic here, not garbage, but outdated. This register exists so that the values aren't garbage (at the cost of being outdated). Except this small notion, I agree on the statement.
>> >> For Z axis, the register bit returns 0 if facing the user, 1 otherwise, >> which the driver translates to 0° and 180° respectively. > > I assume facing up vs facing down? User might be lying on their > back in which case this description doesn't work. The datasheet
Correct, I was playing with the device while seated, hence the bias. But yes, everything is relative to Earth gravity, so face up/down is a better description indeed.
> also talks about the case where g lies near the XY plane and hence > the z axis is horizontal. > > >> >> Those values are proper if the accelerometer is mounted such that the >> XYZ axes are as follows when the device is facing the user in portrait >> mode (respecting the right-hand rule): >> >> y >> ^ >> | >> | >> | >> +----------> x >> / >> / >> / >> L >> z >> >> Since this information is very basic, imprecise (only 4 values for XY >> plane and 2 for Z axis) and can be extrapolated from the actual, >> precise, x, y and z acceleration values, it is not made available >> through buffers. >> >> A change in XY plane or Z axis orientation can also trigger an interrupt >> but this feature is not added in this commit. >> >> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> >> --- >> drivers/iio/accel/mxc4005.c | 39 +++++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c >> index b3afbf064915..61f24058d239 100644 >> --- a/drivers/iio/accel/mxc4005.c >> +++ b/drivers/iio/accel/mxc4005.c >> @@ -20,6 +20,11 @@ >> #define MXC4005_IRQ_NAME "mxc4005_event" >> #define MXC4005_REGMAP_NAME "mxc4005_regmap" >> >> +#define MXC4005_REG_TILT_ORIENT 0x01 >> +#define MXC4005_REG_TILT_ORIENT_Z_MASK BIT(6) > > I think you need to deal with BIT(7) as well. > >> +#define MXC4005_REG_TILT_ORIENT_XY_MASK GENMASK(5, 4) >> +#define MXC4005_REG_TILT_ORIENT_XY_SHIFT 4 > > Don't define the shift, you can use FIELD_GET(MASK, val) >
Wasn't aware of this neat macro, thanks for the heads up.
>> + >> #define MXC4005_REG_XOUT_UPPER 0x03 >> #define MXC4005_REG_XOUT_LOWER 0x04 >> #define MXC4005_REG_YOUT_UPPER 0x05 >> @@ -96,6 +101,7 @@ static const struct attribute_group mxc4005_attrs_group = { >> static bool mxc4005_is_readable_reg(struct device *dev, unsigned int reg) >> { >> switch (reg) { >> + case MXC4005_REG_TILT_ORIENT: >> case MXC4005_REG_XOUT_UPPER: >> case MXC4005_REG_XOUT_LOWER: >> case MXC4005_REG_YOUT_UPPER: >> @@ -214,6 +220,28 @@ static int mxc4005_read_raw(struct iio_dev *indio_dev, >> int ret; >> >> switch (mask) { >> + case IIO_CHAN_INFO_PROCESSED: >> + switch (chan->type) { >> + case IIO_ROT: >> + ret = regmap_read(data->regmap, chan->address, val); >> + if (ret < 0) { >> + dev_err(data->dev, "failed to read rotation\n"); >> + return ret; >> + } >> + >> + if (chan->channel2 == IIO_MOD_X_AND_Y) { >> + *val &= MXC4005_REG_TILT_ORIENT_XY_MASK; >> + *val >>= MXC4005_REG_TILT_ORIENT_XY_SHIFT; > FIELD_GET() > >> + /* 00 = 270°; 01 = 0°; 10 = 90°; 11 = 180° */ >> + *val = (360 + (*val - 1) * 90) % 360; > > In event of tilt not being set (BIT (7)) I think you should return an error > code here. -EBUSY perhaps? To reflect the fact we don't have valid data. >
Difficult to find the appropriate error code to return. It's not technically busy, especially if the device stays in that position forever, it'll never return a valid value.
>> + } else { >> + *val &= MXC4005_REG_TILT_ORIENT_Z_MASK; >> + *val = *val ? 180 : 0; > Documentation for this is really confusing, as it refers to a circumstance > when it can be assumed to be horizontal, but then doesn't define it. > > It might be a simple as tilt being set and thus indicating significant > acceleration due to gravity in the xy plane. > However, the Z orientation is still updated in that case... >
I'm wondering if it's not an exclusive validity of axes. E.g. XY plane is valid only when Z is not and vice-versa?
"The vertical/horizontal Z axis orientation is determined by the same criteria used to determine the XY-plane off-axis tilt angle" seems to indicate that the TILT bit defines whether the Z axis is vertical or horizontal.
"When the XY plane has a sufficiently small off-axis tilt angle, XY orientation detection is valid (and continues to be updated), and the Z axis is detected as horizontal" would mean Z is just not usable when XY orientation is valid (because Z is horizontal and thus does not have a big enough acceleration to be usable).
"When off-axis tilt angle exceeds the threshold discussed above, the Z axis is detected as either vertical up or vertical down, depending on the sign of the Z axis acceleration output." could be interpreted as "when XY orientation is invalid, Z orientation is either vertical up or down".
>> + } >> + return IIO_VAL_INT; >> + default: >> + return -EINVAL; >> + } >> case IIO_CHAN_INFO_RAW: >> switch (chan->type) { >> case IIO_ACCEL: >> @@ -287,11 +315,22 @@ static const unsigned long mxc4005_scan_masks[] = { >> }, \ >> } >> >> +#define MXC4005_CHANNEL_ORIENTATION(_axis) { \ >> + .type = IIO_ROT, \ > > Hmm. Should this be rotation or inclination (so referenced > to gravity). Inclination is not particularly tightly defined but the > point is that it is relative to gravity - kind of a special case of > rot. > > For the adis16209 we handled inclination and rotation. I think rotation > in that device corresponds to XY here. (though it's oddly defined for > X axis, whereas I'm fairly sure it should be Z - as rotation 'about' > z axis). The Z one here should I think be an inclination because it's not > about any particular axis. > > We also have angle to confuse matters. In that case intent was 'between' > two things. Arguably all the uses of rot are as well, just that one of those > things is gravity or magnetic north. With hindsight I think we could have > gotten away with one of them, but hard to tidy up now. >
You mentioned the three candidates I had in mind, but none seemed to perfectly match (or maybe I'm just confused about the difference between the three and just can't make up my mind) so I picked rotation because the English term seemed closer to what I think those values represent?
> In conclusion, what you have here I think is best described as > IIO_ROT about Z axis (the XY one)
I disagree, this would mean that having XY plane parallel to ground and rotate the device along the Z axis (so XY plane staying parallel to ground) should change the value of this IIO_ROT on Z axis, but it does not if I'm not mistaken (I assume because the acceleration on X and Y axes are too small because the axes are parallel to the ground).
But that also kind of highlights that IIO_ROT for Z as I've done it in the patch probably isn't correct either and inclination would probably match best?
I feel like this will be an interesting discussion :)
Cheers, Quentin
| |