Openvario / sensord

Daemon to poll air data from Openvario sensorboard
6 stars 11 forks source link

Fixes Slow Start #30

Closed hpmax closed 3 years ago

hpmax commented 3 years ago

This version has changes to the detection of a glitch, and end of a glitch. It also introduces absolute pressure compensation to the glitch compensation.

Blaubart confirms this seems to address his concerns about it taking up to a minute or two for sensord to start putting out data.

hpmax commented 3 years ago

1) How do I remove the binaries? (I put them in for Blaubart because he was having trouble figuring out how to compile) 2) compdata generates compensation data to deal with the glitch. This is unique to each sensor. You can use the default that I put in the sensord.conf but those are technically unique to my board so it would be preferable to run compdata to get correct values for your board.

I'll work on the other failures.

hpmax commented 3 years ago

Looks like the failure may be that the target name changed from "cal" to "sensorcal" in the Makefile. "Make sensorcal" works just fine. I'm not sure why or when. If I did it, I presume I had a reason... but who knows. Do you want me to change it to cal, or do you want to change the regression suite to look for sensorcal instead?

kedder commented 3 years ago

How do I remove the binaries? (I put them in for Blaubart because he was having trouble figuring out how to compile)

Just remove the commit that adds binaries. Or just add a new commit that removes binaries (if you don't mind this PR to be merged by squashing all commits into one).

There are better ways to distribute binaries, you can just send them by email. Git is not the good way to do that.

compdata generates compensation data to deal with the glitch. This is unique to each sensor. You can use the default that I put in the sensord.conf but those are technically unique to my board so it would be preferable to run compdata to get correct values for your board.

Is it useful after the bug is fixed? Is there any way for anyone to figure out how and when to use it without finding this PR and reading through comments?

hpmax commented 3 years ago

I'm not sure what you mean by "after the bug is fixed." Compdata generates compensation coefficients between the static and TE sensors. This is a hardware issue that won't go away until we get the new hardware. I can add a README explaining usage, but it is fairly straightforward, just: "compdata -c /opt/conf/sensord.conf" and then enter a number (10 should be okay) and wait. Just like with the cal program. This one writes data to /opt/conf/sensord.conf instead of the EEPROM. I'm not sure why we want to store data in the EEPROM as opposed to sensord.conf to begin with.

Regarding code analysis there were a few issues: 1) deltaTIme is initialized but not used, this is an easy enough fix. 2) printf ("%d") should be printf ("%u") easy enough to fix. 3) usleep should be nanosleep... I'm inclined to remove usleep from everything other than (sensor_wait) and then the question is do we change sensor_wait to use nanosleep. I'm a little scared to change this. Do you have a preference or believe nanosleep might work better than usleep? 4) There were a number of complaints about variable scope being excessive. Apparently, my C knowledge is limited to older versions of C which didn't have the concept of scope at other than the function level. I can look into this if anyone cares.

Looking through the code I noticed a number of cases where things may not have been properly initialized, particularly if you are reading data from a file rather than live -- or that data may not be completely consistent between live versus recorded because of how the filter is initialized. I don't think this would cause failure, just maybe some weird behavior when it starts up. I can take a look at this. I'd like to fix things that look like real potential issues, but honestly, this is not something I want to sink too much time in. Going forward, I'd like to be spending my time on the new hardware which is going to require a lot of work and should produce much better results than we can get out of this hardware.

hpmax commented 3 years ago

I am working on a new version which I just pushed that will address some of these issues:

1) Added a README file explaining sensorcal and compdata 2) I removed usleep from everything and replaced it with sensor_wait which is a routine I was already using. sensor_wait itself uses usleep, but if we switch to nanosleep, that means it's all in one place. I also moved sensor_wait into the ms5611.c/h files so that it's common to everything. This should make things a lot cleaner. 3) removed "deltaTime" that wasn't being used. 4) Fixed the printf statement. 5) I noticed that cal is taking 10 datapoints separated by 1 second and averaging them. I think it makes more sense to take 800 data points separated by 12.5ms (which is our actual sampling interval) and average them. That alone, should reduce standard deviation of the error on the offset by 9 times or so. I don't actually understand why this is stored in the EEPROM not the config file, but at this point I don't care.

It's easy enough to change back, but it was using usleep, and I saw no good reason not to average more data.

6) Check buffer boundaries and reduced scope stuff is not addressed. Also, there are two strlen() (that had been in there for a while) which the code checker is complaining about if not 0 terminated.

May be a good idea to switch from usleep to nanosleep in sensor_wait.

hpmax commented 3 years ago

I made a modification to use nanosleep instead of usleep and a few other very minor modifications which I think will make it slightly more consistent in terms of timing. Results look good, almost all the time was either 0.0 or -0.0 m/s, with very occasional blips to -0.1 m/s. However, I am occasionally seeing the vario go nuts. I suspect what happened is:

Previously I was waiting 12.5ms and then reading the pressure sensors and then starting the next conversion. Now I'm waiting 10ms, reading the pressure sensors, and then waiting 12.5ms (from the last conversion) before starting the next conversion. I am suspecting that 10ms is too short a time and is occasionally failing giving bad results.

I changed this because I2C access is actually fairly slow (hundreds of microseconds), so i think I can actually get more consistent timing and hopefully more consistent pressure readings.

I need to ensure this problem is solved and I'd like to compare std deviations to make sure I'm not just fooling myself.

hpmax commented 3 years ago

Please hold off on merging. After running it for some time, I noticed bad readings coming out of the pressure sensor, need to track this down. I'll try to get it done today.

hpmax commented 3 years ago

One of the changes I made (after what I last pushed) seemed to be causing problems. I don't really understand why, and I don't feel like trying to figure it out. I really hate the MS5611.

I made a few more changes, all very minor:

1) Previous version was alternating the order that pressure sensor conversion occurred every 12.5ms. I2C access takes some time, so being consistent probably makes a non-negligible improvement in timing jitter, so I now consistently do TE conversion first, followed by static.

2) I changed the coefficient ranges so that compdata results had a better chance of showing GOOD.

3) I added "glitch_timing" to the config file -- removing some magic numbers from the code and ensuring it's the same between main.c and compdata.c.

4) Made improvements to README

At this point, I am okay with releasing it.

linuxianer99 commented 3 years ago

@hpmax : Can you please have a look at the Codacy Errors: https://github.com/Openvario/sensord/runs/1679814081

https://app.codacy.com/gh/Openvario/sensord/pullRequest?prid=6747280

Would be nice if you fix most of them ... Thanks!

hpmax commented 3 years ago

The issues in 24c16/nmea/sensorcal are all legacy issues. It looks like there are the following types of complaints:

1) Don't use strlen because of the possibility it's not 0 terminated. I understand the concern, but I'm not sure what the alternative is (except maybe to set the last byte to 0 before doing strlen (and I doubt Codacy would consider this a fix anyway). This is a legacy issue.

2) Don't use usleep, use nanosleep instead. I can try to fix this. I moved everything into a single spot to make this an easier fix, but I've had issues with nanosleep in the past and I really don't want to go down unnecessary rabbit holes. This is a legacy issue.

3) "Check buffer boundaries if used in a loop including recursive loops (CWE-120, CWE-20)." Some of these are legacy issues, some aren't, I don't really understand what this is.

4) Reduce scope of variables. Some of these may be legacy issues. When I learned C I don't think there was a concept of block level scope... I'll try to work on this.

Next PR will be to add DS18b20 support per Blaubart's request since it is ready to go PR after that I will try to address some or all of the Codacy issues (but again, I'd appreciate some advice here) PR after that I will add AM2321 temp/humidity sensor per Blaubart's request (I2C based).