Reorder mag return values from XZY order to XYZ. #7

dastels · 2018-10-11T19:41:56Z

No description provided.

jerryneedell · 2018-10-11T20:26:53Z

Looks fine to me.

jerryneedell · 2018-10-11T20:29:01Z

My apologies - I had made a comment but realized it e was due to my own misinterpretation of the code. I deleted the comment which may have been even more confusing.

jerryneedell

Why are the raw magnetometer values multiplied by 16 (line 180) ? This is not a change so has no bearing on the PR, but it is not clear to me why it is done.

caternuson · 2018-10-11T20:58:34Z

I have the same question as @jerryneedell with regards the >>4 (divide by 16) being done in the list comprehension. Why is that being done?

jerryneedell · 2018-10-11T21:29:11Z

Of course I meant divide 😉

dastels · 2018-10-11T22:47:07Z

Not sure about the shifting I just ported the C code, obviously missing that crucial bit about Y & Z being swapped.

dastels · 2018-10-11T23:04:46Z

Double checking the datasheet and C code... it looks like the MAG data doesn't need to be shifted. I'll try it out and submit the change it it makes sense.

dastels · 2018-10-11T23:19:54Z

The driver needs a full once-over IMO, there seems to be plenty of things that are out of sync with the C++ version. When I wrote it I just needed something quick to do what I needed and didn't pay much attention to completeness. I don't have time now, though.

caternuson · 2018-10-11T23:32:15Z

The datasheet really isn't too clear. Unless I'm missing it somewhere, it does not show the actual bit layout for either the accel or the mag registers. Your left with just:

The value is expressed in 2’s complement.

But on the first page, one of the features is "16 bit data output". So I would assume both the accel and mag data are 16 bit 2's complement. The Arduino library has some commented out code dealing with 12 bit values:
https://github.com/adafruit/Adafruit_LSM303/blob/master/Adafruit_LSM303.cpp#L56
but the code that follows is for 16 bit. But I'm also not sure why the mag data isn't cast to a signed int:
https://github.com/adafruit/Adafruit_LSM303/blob/master/Adafruit_LSM303.cpp#L81

Apologies for deviating from the original issue of the PR.

jerryneedell · 2018-10-12T10:34:38Z

interesting taht the Raspberry Pi library does treat the accel data as 12 bit (with the shift) but not the mag data : https://github.com/adafruit/Adafruit_Python_LSM303/blob/master/Adafruit_LSM303/LSM303.py#L81

I this is similar to the Arduino lib.

edited to add - this is not the same as the Arduino -- since the shifts are commented out in the Arduino.
It looks like there should not be any shifting necessary for accel or mag.

jerryneedell · 2018-10-12T12:23:49Z

This really is a strange sensor interface -- the Endianness of the accel and mag are not even the same.... yikes but both are shown as 2's compliment 16 bit values when read. That appears to be how Arduino treats them -- even though it does not cast the mag to int16. I Don't have one of these to test with, but I suggest removing the shifts from the mag conversion and leaving them out of the accel conversion.

jerryneedell · 2018-10-12T12:34:36Z

Gaaa! Its even worse -- the Arduino library referenced above is deprecated! https://github.com/adafruit/Adafruit_LSM303#warning-library-deprecated and the replacement does shift the accel data???? https://github.com/adafruit/Adafruit_LSM303DLHC/blob/master/Adafruit_LSM303_U.cpp#L124
I'm confused....

jerryneedell · 2018-10-12T13:22:20Z

ah -- but there is a factor of 16 difference in the "gain" applied by the CP driver from the "unified" Arduino Driver.
https://github.com/adafruit/Adafruit_LSM303DLHC/blob/master/Adafruit_LSM303_U.cpp#L31
vs

Adafruit_CircuitPython_LSM303/adafruit_lsm303.py

Line 133 in 6f4dbf7

_LSM303ACCEL_MG_LSB = 16704.0

so the accel results are likely the same.

So perhaps only the shifting of the mag data is really different.

Also - the gains need to be looked at closely for the mag to see if they are compensating for the curly applied shift. I don't think they are and I expect that the CP mag data is off by a factor of 16. That won't impact the "compass" directions and I wonder if anyone actually looks at the raw mag data.

jerryneedell · 2018-10-12T13:48:14Z

hmmm -- looking at the example in the guide https://learn.adafruit.com/lsm303-accelerometer-slash-compass-breakout?view=all#circuitpython-and-python-usage-4-6

note that the magnitude of the magnetic field reported is only about 4 microtesla but the earth field should be much larger - more like 50 - 60 microtesla. close enough to a factor of 16 for me to be suspicious.

If you have one of these sensors, it may be worth comparing the Arduino magnitude to the CP magnitude.

brennen · 2018-10-12T17:20:26Z

Is this helpful?

CircuitPython:

>>> import board
>>> import busio
>>> import adafruit_lsm303
>>> i2c = busio.I2C(board.SCL, board.SDA)
>>> sensor = adafruit_lsm303.LSM303(i2c)
>>> print(sensor.magnetic)
(-1.09091, -4.81818, -0.306122)
>>>
>>> print(sensor.raw_magnetic)
(-11, -53, -3)

Arduino, from this sketch:

Driver Ver:   1
Unique ID:    12345
Max Value:    0.00 uT
Min Value:    0.00 uT
Resolution:   0.00 uT
------------------------------------

X: -16.18  Y: -6.09  Z: -85.82  uT
X Raw: -178  Y Raw: -67  Z Raw: -841
X: -16.09  Y: -6.00  Z: -85.71  uT
X Raw: -177  Y Raw: -66  Z Raw: -840

jerryneedell · 2018-10-12T17:37:50Z

looks like a factor of 16 to me ;-)

jerryneedell · 2018-10-12T18:56:44Z

adafruit_lsm303.py

@@ -177,7 +177,8 @@ def raw_magnetic(self):
        """
        self._read_bytes(self._mag_device, _REG_MAG_OUT_X_H_M, 6, self._BUFFER)
        raw_values = struct.unpack_from('>hhh', self._BUFFER[0:6])
-        return tuple([n >> 4 for n in raw_values])
+        values = tuple([n >> 4 for n in raw_values])
+        return (values[0], values[2], values[1])



Given the discussion along with they PR I suggest that the shifts be removed and this simply use:

return (raw_values[0],raw_values[2],raw_values[1])

Or defer that to another issue/PR. We've kind of hijacked this one. The PR fixes what it was meant to fix.

fine with me -- I have no objection to merging this PR.

ladyada · 2018-10-12T20:34:53Z

adding @microbuilder for more details

Updating https://github.com/adafruit/Adafruit_CircuitPython_DotStar to 1.4.0 from 1.3.0: > Merge pull request adafruit/Adafruit_CircuitPython_DotStar#31 from kattni/example-update > Merge pull request adafruit/Adafruit_CircuitPython_DotStar#30 from adafruit/pi_bitbang_exception > Merge pull request adafruit/Adafruit_CircuitPython_DotStar#28 from siddacious/master > ignore the board module imports in .pylintrc Updating https://github.com/adafruit/Adafruit_CircuitPython_LSM303 to 1.2.1 from 1.2.0: > Merge pull request adafruit/Adafruit_CircuitPython_LSM303#7 from dastels/master > ignore the board module imports in .pylintrc > Merge pull request adafruit/Adafruit_CircuitPython_LSM303#6 from kattni/example-update

Reorder mag return values from XZY order to XYZ.

6f4dbf7

kattni requested a review from a team October 11, 2018 19:52

jerryneedell reviewed Oct 11, 2018

View reviewed changes

jerryneedell reviewed Oct 12, 2018

View reviewed changes

caternuson merged commit 2a299cb into adafruit:master Oct 12, 2018

caternuson mentioned this pull request Oct 12, 2018

Need to verify acceleration and magnetic data register layout. #8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorder mag return values from XZY order to XYZ. #7

Reorder mag return values from XZY order to XYZ. #7

dastels commented Oct 11, 2018

jerryneedell commented Oct 11, 2018

jerryneedell commented Oct 11, 2018

jerryneedell left a comment •

edited

Loading

caternuson commented Oct 11, 2018

jerryneedell commented Oct 11, 2018

dastels commented Oct 11, 2018

dastels commented Oct 11, 2018

dastels commented Oct 11, 2018

caternuson commented Oct 11, 2018

jerryneedell commented Oct 12, 2018 •

edited

Loading

jerryneedell commented Oct 12, 2018 •

edited

Loading

jerryneedell commented Oct 12, 2018

jerryneedell commented Oct 12, 2018 •

edited

Loading

jerryneedell commented Oct 12, 2018 •

edited

Loading

brennen commented Oct 12, 2018 •

edited

Loading

jerryneedell commented Oct 12, 2018

jerryneedell Oct 12, 2018

caternuson Oct 12, 2018

jerryneedell Oct 12, 2018

ladyada commented Oct 12, 2018

Reorder mag return values from XZY order to XYZ. #7

Reorder mag return values from XZY order to XYZ. #7

Conversation

dastels commented Oct 11, 2018

jerryneedell commented Oct 11, 2018

jerryneedell commented Oct 11, 2018

jerryneedell left a comment • edited Loading

Choose a reason for hiding this comment

caternuson commented Oct 11, 2018

jerryneedell commented Oct 11, 2018

dastels commented Oct 11, 2018

dastels commented Oct 11, 2018

dastels commented Oct 11, 2018

caternuson commented Oct 11, 2018

jerryneedell commented Oct 12, 2018 • edited Loading

jerryneedell commented Oct 12, 2018 • edited Loading

jerryneedell commented Oct 12, 2018

jerryneedell commented Oct 12, 2018 • edited Loading

jerryneedell commented Oct 12, 2018 • edited Loading

brennen commented Oct 12, 2018 • edited Loading

jerryneedell commented Oct 12, 2018

jerryneedell Oct 12, 2018

Choose a reason for hiding this comment

caternuson Oct 12, 2018

Choose a reason for hiding this comment

jerryneedell Oct 12, 2018

Choose a reason for hiding this comment

ladyada commented Oct 12, 2018

jerryneedell left a comment •

edited

Loading

jerryneedell commented Oct 12, 2018 •

edited

Loading

jerryneedell commented Oct 12, 2018 •

edited

Loading

jerryneedell commented Oct 12, 2018 •

edited

Loading

jerryneedell commented Oct 12, 2018 •

edited

Loading

brennen commented Oct 12, 2018 •

edited

Loading