Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for tilt calculation error #410

Merged
merged 1 commit into from
Nov 10, 2020
Merged

Conversation

tomsykes
Copy link
Contributor

@tomsykes tomsykes commented Aug 6, 2020

This is a fix for the tilt calculation equation(s)

please see #409 for details of the problem and fix

Fix tilt calculation
@universam1
Copy link
Owner

@tomsykes Thank you for the effort of providing a “fix”. However before accepting a change like the I’d need to see why your formula is actually correct. You describe that you came up with a better one but there is no explanation why this is actually correct.

@tomsykes
Copy link
Contributor Author

tomsykes commented Aug 6, 2020

Details of the problem are in the issue... #409

@tomsykes tomsykes changed the title Update iSpindel.cpp Fix for tilt calculation error Aug 10, 2020
@jpgimenez
Copy link

I found this because I was looking at a problem with my iSpindel, I'm re calibrating my device and I found that after calibration if I put it in vertical position it reports ~60º, the only way to fix that is calibrating with the battery looking up, after that it reports ~3º in vertical position and ~27º in plain water. Maybe I'll test this to see if it helps with my issue.

@stale
Copy link

stale bot commented Oct 17, 2020

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.

@stale stale bot added the wontfix label Oct 17, 2020
@willcl-ark
Copy link

Odd to have such little interest in fixing a fundamental incorrect calculation?

@stale stale bot removed the wontfix label Oct 19, 2020
@thegreatgunbantoad
Copy link
Contributor

@universam1 Would just adding a comment before the calculation of something like:
//Simplified tilt calculation based on the assumption that the device is at rest. Note: https://www.nxp.com/docs/en/application-note/AN3461.pdf Equation 55
cover the explanation

@tomsykes
Copy link
Contributor Author

Equation 55 isn't an equation being used anywhere in the original code or my fix, so I'm not sure why a comment referencing it would be helpful.

@ErikdBr
Copy link
Contributor

ErikdBr commented Nov 10, 2020

Can anyone explain to me in layman's language why this fix is necessary in the first place because an iSpindel in fluid doesn't roll, it just tilts, nothing else.

@tomsykes
Copy link
Contributor Author

tomsykes commented Nov 10, 2020

Sure, there are any number of reasons the iSpindel might be rolled when compared with the position it was at when initially calibrated, but a couple of examples...

  1. yeast/trub and CO2 bubble attach themselves to the ispindel during fermentation. This causes some deviations in both pitch and roll... the change in pitch is something we have to live with, but we don't want the change in roll to affect the calculated tilt even further.
  2. everytime I take my ispindle out of the tube... if I put it back in at a different orientation, the inconsistencies in the tube mean that it floats at a slightly different roll angle.

They're small amounts of roll, but they're there, and they're affecting the calculated tilt angle. Is there any reason we should ignore an error, just because we think the effect is minimal? I don't think so. The existing calculation is wrong, we have a fix, why would we not apply it?

Even if you don't think the roll is enough to cause you any problems (after all, this is not a high precision thing)... someone might stumble across the code and copy the tilt calculation. We're in a position to make an improvement, we've all spent more time commenting on it than it would take to confirm my calculation and accept the PR. What's the beef?

@Scales82
Copy link

we've all spent more time commenting on it than it would take to confirm my calculation and accept the PR. What's the beef?

AMEN! Testify!

@thegreatgunbantoad
Copy link
Contributor

Equation 55 isn't an equation being used anywhere in the original code or my fix, so I'm not sure why a comment referencing it would be helpful.

Equation 55 gets you to the equation you used, granted it stops at cos(tilt) and doesn't take the arc cos or convert from rad to deg.

@tomsykes
Copy link
Contributor Author

tomsykes commented Nov 10, 2020

Apologies, yes, you're quite right. I'm afraid it has been some time since I actually looked at the code, and my memory of the calculation was very different.


Random thought, essentially shouldn't be part of this PR or related issue...

If this were being done in the best possible way... we'd actually do the zero tilt calibration in pure water using this full 3 axis tilt calculation, rather than on a flat surface. Tilt could then be calculated based on the difference from calibration measurement. We'd also be taking temperature in to account.

Obviously, this would require re-measuring for the polynomial, and it would have very different numbers, but everything else should still work.

The reason I suggest this is that calibrating on a flat surface is just bringing another inaccuracy in to the mix... as well as not taking in to account the fact that a flat surface is not the position or medium in which we're taking our real life tilt measurements. My guess is that it's entirely likely that the centre of gravity with two points touching a surface is not the same as the centre of buoyancy. However, I suspect this kind of change is more than anyone here is willing to accept as the hassle of re-measuring the polynomial probably outweighs the benefits.

@ErikdBr
Copy link
Contributor

ErikdBr commented Nov 10, 2020

...What's the beef?

No beef, just wondering. After your explanation I can see it is an improvement so I can see no obstacles to implement it. However, it needs another review before it can be merged. If someone with knowledge of coding could review it and can confirm that it is ok then it is merged automatically? I am afraid @universam1 is the only one who can merge stuff without a review as he is the owner of this project.

Copy link
Contributor

@ErikdBr ErikdBr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far I can see the coding is ok.

@tomsykes
Copy link
Contributor Author

@ErikdBr Didn't you report that you couldn't repeat the problem in the first place? Perhaps there was a miscommunication and you were reporting the values using my fix?

My PR fixes it for me, and clearly for a couple of other people too... but it might be prudent to get @ErikdBr to test that the new code isn't a regression for him/his hardware configuration before we merge. At this stage, I think it should be the only blocker.

Copy link
Contributor

@thegreatgunbantoad thegreatgunbantoad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This updated calculation can be seen to be in keeping with https://www.nxp.com/docs/en/application-note/AN3461.pdf Equation 55

Code has been built and tested and this change has been seen to work.

@ErikdBr
Copy link
Contributor

ErikdBr commented Nov 10, 2020

@ErikdBr Didn't you report that you couldn't repeat the problem in the first place? Perhaps there was a miscommunication and you were reporting the values using my fix?

My PR fixes it for me, and clearly for a couple of other people too... but it might be prudent to get @ErikdBr to test that the new code isn't a regression for him/his hardware configuration before we merge. At this stage, I think it should be the only blocker.

I did report I myself had no issues using the 6.3.1 firmware without your fix. However, when I roll it on a flat surface as in your video, I have the same result as you had in that video. When I rolled it at an angle as if it would be in a fluid I saw minor differences in the tilt, in my opinion not enough as it would not roll that much in fluid in batches that I make was my thought.
So assuming that the iSpindel does not roll in a fluid that much I didn't see any problem, all the more because I only use it to see if the fermentation is done and not to know an exact SG, I update the polynominal after every batch so it keeps getting more precise but the final measurement I do by hand.

So for me personaly it wouldn't make much of a difference but if the tilt and thus the measured SG could be more precise it is in everybody's interest.

If someone could make a firmware.bin file for me with these changes in it, I would be happy to test it for you with my hardware.

@thegreatgunbantoad
Copy link
Contributor

It's my understanding that sharing any unofficial .bin files would breach the software licence. It's not too difficult to setup VS code and Platform I/O but it is a bit of a faff.

@tomsykes
Copy link
Contributor Author

tomsykes commented Nov 10, 2020

It does appear that that is the case... nice example of someone trying to prevent a fork from becoming the choice for everyone...

Although it strikes me that the license is at direct odds with how forking a project on github works, and is probably in direct contravention of their own terms.

The more you tighten your grip, Tarkin, the more star systems will slip through your fingers

@universam1
Copy link
Owner

This updated calculation can be seen to be in keeping with https://www.nxp.com/docs/en/application-note/AN3461.pdf Equation 55

Code has been built and tested and this change has been seen to work.

Thank you @thegreatgunbantoad for this kind of valuable feedback I was looking for. Good work @tomsykes for bringing up that improvement

@universam1 universam1 merged commit c0c8447 into universam1:master Nov 10, 2020
@ErikdBr
Copy link
Contributor

ErikdBr commented Nov 10, 2020

So now I see a pre-release, no bin file. When will there be a bin file to download, how does this process work? ( I am eager to test it out)

@thegreatgunbantoad
Copy link
Contributor

@universam1 Don't forget to update the firmware version in Globals.h when you decide to up version. Very easy to miss that is.

@universam1
Copy link
Owner

universam1 commented Nov 10, 2020

Pipeline now finished the build, the binary is available. Pre-release for now, releasing when confirmed

@thegreatgunbantoad
Copy link
Contributor

Pipeline now finished the build, the binary is available. Pre-release for now, releasing when confirmed

Tested, code looks to work but Firmware version is still reporting as 6.3.1

@ErikdBr
Copy link
Contributor

ErikdBr commented Nov 10, 2020

Pipeline now finished the build, the binary is available. Pre-release for now, releasing when confirmed

Tested, code looks to work but Firmware version is still reporting as 6.3.1

+1 Tested, Firmware version is still reporting as 6.3.1

@Scales82
Copy link

Scales82 commented Nov 10, 2020

I think the globals.h needs updating as per @thegreatgunbantoad said.. As its a pre-release (as i understand), probably no official version number to place in yet.

@tomsykes
Will end-users require a new flat-surface cal. or sugar-water calibration after flashing this new pre-release?

@thegreatgunbantoad
Copy link
Contributor

@Scales82 as far as I can see, horizontal is the the only thing that makes sense as a calibration point as it's as close to an absolute as you can get unless you do pure vertical. How cloud you possibly tell what angle the thing floats at with measuring that by some other means? Might be missing something though.

@Scales82
Copy link

@Scales82 as far as I can see, horizontal is the the only thing that makes sense as a calibration point as it's as close to an absolute as you can get unless you do pure vertical. How cloud you possibly tell what angle the thing floats at with measuring that by some other means? Might be missing something though.

Think you may have mis-interpreted my question. I was simply asking if people whom have already configured and calibrated their spindels will require to re-do the process from scratch after carrying out the proposed new FW?

@ErikdBr
Copy link
Contributor

ErikdBr commented Nov 11, 2020

The fix doesn't seem to improve anything, in fact it makes things worse for me. I rolled it 360 degrees at a starting tilt angle of 43-ish, at 90 degrees roll I had a tilt angle of 50 degrees. The video shows the info page as I did the 360 degrees roll.
@tomsykes Any advise?

@thegreatgunbantoad
Copy link
Contributor

@Scales82 as far as I can see, horizontal is the the only thing that makes sense as a calibration point as it's as close to an absolute as you can get unless you do pure vertical. How cloud you possibly tell what angle the thing floats at with measuring that by some other means? Might be missing something though.

Think you may have mis-interpreted my question. I was simply asking if people whom have already configured and calibrated their spindels will require to re-do the process from scratch after carrying out the proposed new FW?

Oh, I see. I wouldn't think you need to do anything with the poly just re-cal the sensor flat.

@Scales82
Copy link

Works great on my tests. angle stays true no matter the amount of roll I apply. not sure what you are doing @ErikdBr .

@vytux-com
Copy link

How straight in the tube is the PCB?

I would guess that some are fairly centred and thus not showing any errors others might be at an angle so if you roll the tube you will get varying angles of tilt even though the external tube itself is not being tilted

@ErikdBr
Copy link
Contributor

ErikdBr commented Nov 12, 2020

Works great on my tests. angle stays true no matter the amount of roll I apply. not sure what you are doing @ErikdBr .

The same you do, I wil test it again later on with an update of the firmware after a complete initialize of the memory of the Wemos. Maybe that helps.

@ErikdBr
Copy link
Contributor

ErikdBr commented Nov 12, 2020

How straight in the tube is the PCB?

I would guess that some are fairly centred and thus not showing any errors others might be at an angle so if you roll the tube > you will get varying angles of tilt even though the external tube itself is not being tilted

That is what this update should fix. Also I measured it, it is straight in the tube.

@thegreatgunbantoad
Copy link
Contributor

@universam1 @tomsykes I think there's another place in the code where the same correction should be made: Lines 1233-1235

@thegreatgunbantoad
Copy link
Contributor

@universam1 @tomsykes I think there's another place in the code where the same correction should be made: Lines 1233-1235

Also I think line 92 can now be removed

@universam1
Copy link
Owner

@universam1 @tomsykes I think there's another place in the code where the same correction should be made: Lines 1233-1235

Also I think line 92 can now be removed

welcome to open a PR

@thegreatgunbantoad
Copy link
Contributor

@universam1 @tomsykes I think there's another place in the code where the same correction should be made: Lines 1233-1235

Also I think line 92 can now be removed

welcome to open a PR

Will do

@Scales82
Copy link

That is what this update should fix.

That's not my understanding, if there is a tilt from horizontal plane the spindel will still recognise it. It is simply the roll axis that is eliminated. If your board is on a slight angle from 'flat' in the tube and you roll it, that angle will still change post-fix. Mine varys slightly throughout the roll, but nowhere near as bad as it did before.

@ErikdBr
Copy link
Contributor

ErikdBr commented Nov 13, 2020

That is what this update should fix.

That's not my understanding, if there is a tilt from horizontal plane the spindel will still recognise it. It is simply the roll axis that is eliminated. If your board is on a slight angle from 'flat' in the tube and you roll it, that angle will still change post-fix. Mine varys slightly throughout the roll, but nowhere near as bad as it did before.

You are absolutely wright, but this fix hasn't changed anything for me, in fact it varies more then before. Probably because of the finding by @thegreatgunbantoad of a part in the code where the same fix should have been applied.

universam1 pushed a commit that referenced this pull request Nov 13, 2020
* Clean up following item #410

Remove unused variables
Updated Tilt calc on line 1232 in line with item #410

* Upversion firmware ID
@ErikdBr
Copy link
Contributor

ErikdBr commented Nov 13, 2020

The l

@universam1 @tomsykes I think there's another place in the code where the same correction should be made: Lines 1233-1235

Also I think line 92 can now be removed

welcome to open a PR

Will do

I just tested the latest 6.4.1 and that did the trick! Thanks to @thegreatgunbantoad for this fix of @tomsykes and thanks to @tomsykes for the initial improvement and triggering all this and last but not least to @universam1 for his work.

bdelbosc pushed a commit to bdelbosc/iSpindel that referenced this pull request Nov 14, 2020
Improved tilt calculation
bdelbosc pushed a commit to bdelbosc/iSpindel that referenced this pull request Nov 14, 2020
* Clean up following item universam1#410

Remove unused variables
Updated Tilt calc on line 1232 in line with item universam1#410

* Upversion firmware ID
@Scales82
Copy link

So to be clear... This wont effect someones polynomial at all? just flash, re-set the zero on flat surface, and should be g2g?

@tomsykes
Copy link
Contributor Author

Sorry for recent silence on this issue. I started a new project at work which has been consuming a lot of my time...

So to be clear... This wont effect someones polynomial at all? just flash, re-set the zero on flat surface, and should be g2g?

Working under the assumption that the new formula will either make no difference, or a slight improvement to the accuracy of a tilt measurement, your existing polynomial will still work and be as accurate as it was before. Re-setting your level calibration isn't strictly necessary either, but since it's so simple to do, it's worth it.

There's a slight chance that re-calculating your polynomial will give you a small improvement in accuracy.

In the interests of science, I would be interested to know the results/differences if anyone did decide to recalculate their polynomial. I personally calculated my one and only polynomial with the replacement formula, so haven't got two sets of results to compare.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants