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

feat(sensors): Self baselining during sensor moves #786

Merged
merged 26 commits into from
Jun 26, 2024

Conversation

ryanthecoder
Copy link
Contributor

We baseline the sensor reading during moves so that we can compensate better for different tips having a different baseline pressure differential when the plunger is moving

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Would love to see some constant definitions, and I'm a bit confused about the 10 sample windowing

@@ -290,10 +293,21 @@ class MMR920 {
}

void send_accumulated_sensor_data(uint32_t message_index) {
Copy link
Member

Choose a reason for hiding this comment

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

does this overlap with #785 ?


sensor_buffer_log(response_pressure);

if (sensor_buffer_index == 10 && !crossed_buffer_index) {
Copy link
Member

Choose a reason for hiding this comment

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

this 10 is setting the baselining window size, right? let's drop that in a define or a constexpr or something


if (sensor_buffer_index == 10 && !crossed_buffer_index) {

current_pressure_baseline_pa =
Copy link
Member

Choose a reason for hiding this comment

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

why are we incrementing instead of setting this? won't this make it run away? also we could put 10*current_pressure_baseline_pa in the init argument to accumulate if we do want the accumulation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment that explains why this won't run away

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re-worked it to make it more clear

@ryanthecoder ryanthecoder force-pushed the EXEC-515-self-baselining branch from 0901b67 to 7a26107 Compare June 25, 2024 18:52
@ryanthecoder ryanthecoder force-pushed the EXEC-515-self-baselining branch from 50e5b5f to ecd943b Compare June 25, 2024 20:49
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.62%. Comparing base (34c52a3) to head (ecd943b).
Report is 21 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #786      +/-   ##
==========================================
- Coverage   83.28%   82.62%   -0.67%     
==========================================
  Files         101      102       +1     
  Lines        4063     4133      +70     
==========================================
+ Hits         3384     3415      +31     
- Misses        679      718      +39     

see 11 files with indirect coverage changes

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Okay, looks good to me. We may want to gate autobaselining behind a separate command, but we can do that in a different PR if you want to merge this one.

@ryanthecoder ryanthecoder merged commit f0a0fb8 into main Jun 26, 2024
38 of 39 checks passed
ryanthecoder added a commit that referenced this pull request Jul 18, 2024
* Use logic or instead of hardcoded enums

* when filling the bufffer auto baseline after the first 10

* the new method that lets us increase senstivity uncovered an issue with setting the sync line this way, it was turning off before the head node could detect it and stop

* few tweaks to the way this works

* only compute the first 10 elements to self-baselien

* don't immediatly turn of the recording when move completes

* save response pressure

* fix the pressure leveling

* rebase fixups

* fix the hardware delay hack and get rid of another instance of it

* send ack after sending buffer

* fixes to the circular buffer

* don't need this and can cause the process to choke

* send the can messages faster

* format

* make the moving baseline log clearer

* use the real world sensor speed

* change auto baseline slightly to ignore the first N samples

* add a new sensor sync value

* don't trigger before autobaseline is finished

* add new binding type for sensors and use the auto-baselineing during sensor moves

* forgot to change an old reference

* format lint

* preserve old behavior

* reduce the complexity in the handle function to satisfy lint

* format
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.

3 participants