Updates to DIY-Flow-Bench.ino and calibration.cpp

[KirikauKiwi]

Changes to the formulas used for the calibration and user offsets, so negative and positive values are applied correctly.
Currently a positive value decreases the offset and a negative value increase the offset.
Now when a negative value is applied it will reduce the offset and a positive value will increase the offset.

[DeeEmm]

So we already had the offsets set up the way that you are proposing, but they were changed to the current method.

#178 (comment)

When we do the leak calibration. if there is a leak, the displayed flow value will be larger than the actual value as it is taking in extra air. So our 14.7cfm calibration orifice will show a flow rate of 14.7cfm + the leak amount. Lets call that leak 5cfm.

That 5cfm is stored in the leak_calibration field when we calibrate.

Then when we view our measured flow value , we need to subtract the leak amount from the displayed flow value so that it corrects the result. i.e. we are reading flow+leakage (14.7+5). So to get back to correctly reading flow, we need to subtract the leakage that we previously saved. So (Flow+leakage)-leak_calibration = Flow (14.7+5-5)

This is the same for the baseline.

It is important to remember all flow values are displayed as +ve values as we convert negative to positive using the ABS function so we never see a -ve flow value in the UI

We also have reverse flow leak calibration fields and we can detect reverse flow using the +ve/-ve depression values. But at present we are only looking at forward flow.

So I don't believe that this 'fix' is needed, but will concede that reverse flow code still needs to be sorted out. However we must specifically identify reverse flow and deal with that use case on its own.

[KirikauKiwi]

Yes I understand what you say, I was basing it on what we would see on the GUI as you say we will see a value that is higher than we expect for the calibration orifice.

I based my logic on that if I want to reduce a number remembering the end user cannot see the formula used, using your figures above we would see 19.7 CFM on the GUI if I wanted to reduce it would I do that with a 7 or a -7. I also think it will make sense more when you look at the config page and see -7 that you know there is a negative offset applied to the displayed value. Would you expect to read a bank statement that just said $7 and not know if you were in debt or credit.
I always try to think what would be the most usual or commonly expected way for the end user to do something.

Also what would happen if you needed to add to the offset, say if a MAF sensor reads low for a calibration orifice, e.g you get a reading of 12" inH2O when the expected is 14.7 inH2O, to me it does not seem logical to then have to enter -7 as the offset for the current calculation to show an increase on the GUI, puts us in the land of double negatives.

[DeeEmm]

Ok I see what you are saying

I need to have a ponder over this one.

The original intent was that calibration was not a manual process. You may recall that originally the calibration values were greyed out. They were only there for information purposes only. So the stored value just represented the differential.

I will take a deeper look at this once I've addressed #217

[DeeEmm]

This does not work

Previously we created a differential that we apply as an offset to all readings

sensorVal.FlowCFM - config.cal_flow_rate
actual flow - target flow 
14cfm - 14.7cfm = -0.7cfm

now we have

actual flow + target flow 
14cfm + 14.7cfm = 28.7cfm

[DeeEmm]

There are a lot of code changes required just to flip the signs on the values some of which have been missed in this PR. There has been a lot of work undertaken to get the calibration working and this PR currently breaks that. So I think at this stage as we have working code and this PR contains breaking changes I propose that this change is not made at this time.

I do not see that there is a large benefit to this change at this stage, especially if it is going to take an amount of work to fix and test. So in terms of cost / reward I feel that there are other changes that should really get priority.

I propose we park this as a feature request and revisit it as part of V3 once V2 is finalised.