Incorrect maths used in tilt calculation

tomsykes commented 4 years ago

I'm submitting a ...

[x] bug report
[ ] feature request
[ ] support request => Please note that this repositories issues are intended for feature requests and bug reports.

Did you follow the general troubleshooting steps first:

[x] Follow through the FAQ ?
[x] Checked the logs for errors in the terminal console ?
[x] Searched for similar reports already ?

Report

The maths used in void calculateTilt() is not actually calculating tilt correctly.

When the tube has a roll value of 0°, the tilt is calculated accurately. When the tube has a roll of anything other than 0°, the tilt is not accurate.

This might explain a few of the calibration errors people have been reporting and the need for the accelerometer to be soldered very precisely (it is a 3-axis accelerometer after all, and should work the same at any offset angle).

The problem is actually as described by @TTornblum in https://github.com/universam1/iSpindel/issues/358

Steps to Reproduce (for bugs)

The key to understanding the problem is to first understand what we're supposed to be measuring, and what pitch and roll are. I'm not going to explain these things here, but assume a general understanding of the differences.

Here's a zip file containing two videos iSpindel tilt error videos.zip

the first (iSpindel 6.3.1.mp4) shows how the tilt angle is incorrectly calculated, and drops as low as about 65°, when the tilt should remain around 90°
the second (iSpindel tilt fix.mp4) shows the correct tilt calculation being used (PR to follow). Notice that when horizontal, but rolled... the tilt angle remains at around 90°

Detailed explanation

There are actually two problems with the current calculations.

Pitch/roll

One of the existing pitch and roll calculations (I think it was pitch) was being affected by the other dimension (ie, if you roll the board, the pitch and the roll were both being affected, which shouldn't happen).

  float pitch = (atan2(_ay, sqrt(_ax * _ax + _az * _az))) * 180.0 / M_PI;
  float roll = (atan2(_ax, sqrt(_ay * _ay + _az * _az))) * 180.0 / M_PI;

Pitch should be as follows (with sign adjustment for first argument in roll)

  float pitch = atan2(_ay, _az) * 180.0 / M_PI;
  float roll = atan2(-_ax, sqrt(_ay * _ay + _az * _az)) * 180.0 / M_PI;

Tilt

Additionally, the calculation to combine pitch and roll to get tilt is incorrect. It's possible to have both pitch and roll values of 90°, which would make this come out at ~127° (this was masked by the incorrect pitch calc above, which ensured this tilt calculation would never go above 90°.

  return sqrt(pitch * pitch + roll * roll);

I don't have a definite fix for this calculation, as I've got a simpler calculation that doesn't need an intermediate pitch/roll (this is the calculation used in the second video)...

  return acos(_az / (sqrt(_ax * _ax + _ay * _ay + _az * _az))) * 180.0 / M_PI;

As mentioned PR to follow.

References: https://www.digikey.com/en/articles/using-an-accelerometer-for-inclination-sensing https://engineering.stackexchange.com/a/22182

Your Environment

Firmware Version used: 6.3.1

allthepies commented 4 years ago

Wow! I implemented the "fix" and flashed to an ISpindel and also get ~90 degrees on a flat surface regardless of rolling on that plane.

tomsykes commented 4 years ago

Wow! I implemented the "fix" and flashed to an ISpindel and also get ~90 degrees on a flat surface regardless of rolling on that plane.

Sorry if I'm being stupid, but while you seem to be agreeing with my results, I can't quite work out if you're agreeing with me that the existing calculation is giving incorrect tilt values, or if you think my results are the correct values.

allthepies commented 4 years ago

Wow! I implemented the "fix" and flashed to an ISpindel and also get ~90 degrees on a flat surface regardless of rolling on that plane.

Sorry if I'm being stupid, but while you seem to be agreeing with my results, I can't quite work out if you're agreeing with me that the existing calculation is giving incorrect tilt values, or if you think my results are the correct values.

I don't know if the current calculation is giving incorrect tilt values but I am agreeing with you in that your revised calculation looks more robust (than the current one).

brendongough commented 4 years ago

This looks promising, very nice find and report.

universam1 commented 4 years ago

@tomsykes Thank you for creating this new formula, it is however somewhat a black box to me. In order to make that maintainable can you please shed some more light into the algebra how you came up to

tomsykes commented 4 years ago

@universam1 I did include two reference links in my original post. They are both quite detailed. The digikey one was what I based it all on - specifically the section titled "triple axis tilt calculation", but the rest of the article is useful for background.

Here again, just in case there was a problem with the first one... https://www.digikey.com/en/articles/using-an-accelerometer-for-inclination-sensing

I also notice that the PDF link out from the stack exchange answer is broken... including it here for those who are interested - of particular interest in this one are the error maps - worth a look. https://www.nxp.com/docs/en/application-note/AN3461.pdf

Scales82 commented 4 years ago

This seems very promising. I noticed it was implemented but then removed at some point? @universam1 are you planning to put it back into the code? Feedback on FB was promising.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

tomsykes commented 4 years ago

The iSpindel has one primary function - to measure specific gravity. To do that it must measure it's tilt angle, with respect to the horizontal.

I've provided some good evidence that the tilt calculation is wrong (look at the videos in my original post), and have provided a fix - with references to two places that corroborate the new calculation I've proposed. I've also provided videos that back it up.

How is it that there is absolutely no interest in it from the project owners?! Baffling.

Perhaps someone could explain what's needed for this issue to be taken seriously?

allthepies commented 4 years ago

Yes, I was very surprised to see so little interest. I flashed my iSpindel with the revised calculation and verified it's accuracy independent of horizontal roll. I guess I'll have to fork the codebase, incorporate the modification and use that from now on.

vytux-com commented 4 years ago

When someone turns on a stalebot it's usually a sure sign that they don't care about the project anymore

ErikdBr commented 4 years ago

I have just build several iSpindels and have no issues, when I roll it 180 degrees on a flat surface it gives the same tilt result namely 90-ish degrees as it should.

tomsykes commented 4 years ago

That's really intriguing @ErikdBr ... are you using any of the standard PCBs like a CherryPhillip? Are you using the latest firmware revision?

Perhaps you have your accelerometer oriented differently, or it's swapped two of the xyz readings? What happens when you rotate it around the other horizontal axis?

ErikdBr commented 4 years ago

I am using: Original Lolin ESP8266 D1 Mini V3.1.0 The iSpindel @Mikmonken v2 circuit board from pcbs.io and a Schottky BAT43 Diode 200mA 3 instead of the 390Ohm resistor

When I rotate around the other horizontal axis I get a tilt of close to zero.

tomsykes commented 4 years ago

@ErikdBr would you mind running an experiment for me?

Could you make a note of the tilt angle reported by one of your iSpindels when tilting it between 0 and 90 degrees from the horizontal... possibly trying to see what's reported a 0, 22.5, 45, 67.5 and 90 degrees. Then roll the device by a small amount and repeat? Maybe getting 4 or 5 roll positions between 0 and 90 degrees of roll. It's not too important to know exactly how much roll is involved, but I'm keen to see if you're getting consistent tilt angles at different roll angles.

ErikdBr commented 4 years ago

At 42 horizontal it shifts to 47 when rolling 90 degrees, This is the same for the other horizontal angles, it shifts about +5 degrees.

But that shouldn't be an issue at all as the extra weight you have to put in the bottom of the iSpindel , in your case the weight is the battery I guess, keeps it from rolling in fluid.

Scales82 commented 4 years ago

The iSpindel has one primary function - to measure specific gravity. To do that it must measure it's tilt angle, with respect to the horizontal.

I've provided some good evidence that the tilt calculation is wrong (look at the videos in my original post), and have provided a fix - with references to two places that corroborate the new calculation I've proposed. I've also provided videos that back it up.

How is it that there is absolutely no interest in it from the project owners?! Baffling.

Perhaps someone could explain what's needed for this issue to be taken seriously?

I share your pain mate. I think its a great thing you've done and VERY informative. It was implimented and then removed was it not? I remember there being a new version deployed very briefly but I did not have a chance to download it, then when I got home it was gone.

Is there a way to build your changes into a custom FW? Im not very good at this stuff.

tomsykes commented 4 years ago

That shouldn't be a problem, @Scales82... but I won't be able to get on it this week. I'll post the firmware image on my fork when I've got it ready.

thegreatgunbantoad commented 4 years ago

If someone forks this and provides updated code with good comments for understanding I suspect it would be merged into the main project.

I'll be straight here I only skimmed very quickly, but looking at: https://www.nxp.com/docs/en/application-note/AN3461.pdf Assuming the thing is not moving, isn't there a simpler way to measure tilt where it's just:

thegreatgunbantoad commented 4 years ago

@tomsykes

Replacing: return sqrt(pitch * pitch + roll * roll);

with

return acos( _az / ( sqrt( (_ax * _ax) + (_ay * ay) + (_az * _az) ) ) ) * 180.0 / M_PI;

Seems to work well and removes the need to calc pitch and roll in the float calculateTilt() function.

If that fits with your understanding of how things are calculated I'm fine to do a pull request to have it added for you.

tomsykes commented 4 years ago

Am I misunderstanding something?! I created a pull request along with this issue. It's linked below my first comment.

tomsykes commented 4 years ago

I'll link it again, since you obviously all missed it https://github.com/universam1/iSpindel/pull/410

Scales82 commented 4 years ago

@tomsykes I'm also at a loss here. You have handed it all up on a platter.... Not sure whats going on.

thegreatgunbantoad commented 4 years ago

@tomsykes Sorry my skim reading missed a bunch of your info, my bad

universam1 commented 4 years ago

I'm happy to accept improvements like this @tomsykes - just as I said the form of documentation was hardly maintainable.

Since we have now more insights into the details provided by this discussion here and other testers, we can go on to release it

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

universam1 / iSpindel