Fixed problems when decimal shifts != 0

[rainman110]

The main reason was a wrong digital / analog sync, if decimal shifts were != 0.

The fix is not beautiful - I temporally revert the decimal shift to make it easier to sync the analog and digital part.
At the end, it is applied again. It works though.

I also fixed hanging digits for late transition, which happened at my own installation only.
Also reverted back to the improved unit test system of @Slider0007, which seemed to be mistakenly overwritten by someone.

Fixed #2857 and Discussion #2243

@Slider0007 Please have a look, in particular at test_suite_flowcontroll.cpp. I went back to your testing setup. Not sure, why it was the old code again?!

[Slider0007]

@rainman110: That's great for the affected users you figured it out :-)
Unit test system seems OK again. Thanks for fixing it. It seems it was accitentally overwritten while merging a PR after my PR: #2783 (comment)

General questions/remarks (without any rating):

• Are you still using results from 'old' sync logic, meaning there are now two logics which are doing sequenceally almost the same?
• How do differenacte that you accidentally not adjust twice?
• Wouldn't it be better to do it either the one way or the other way do avoid confussion? I'm not the judge ;-)
• You use some constants which differ from 'old' logic in place, e.g. hanging digit compensation ('old': >= x.8, 'new': >= x.9)
• It seems there are now quite different understanding between early and late (Interpretation up to now: https://jomjol.github.io/AI-on-the-edge-device-docs/Watermeter-specific-analog---digital-transition/).
  • From logic point of view parameter Analog/DigitalTransisitionStart is more or less coded like 'Analog number value when digit reaches digit uncercainty (>= x.8) to decide if compensate hanging digit or interprete as early arrived digit in uncertainty area. If digit > x.8 and analog higher than threshold it's not handled as hanging digit but as early digit.
  • In my opinion 'AnalogDigitTranisistionStart' is not the perfect fitting name, maybe more generic would be better 'AnalogDigitSyncValue'.
  • @haverland @jomjol: If I understood/stated something wrong, please feel free to correct.

---

Further questions (just for my understanding as I want to integrate the new test cases in my environment with derived 'old' logic as well):

• I do not understand some (most) of your new test cases. Maybe I did not get your interpretation of 'a2dt' correctly...

float a2dt = 3.6;
// meter shows 012.4210 , this is after transition
TEST_ASSERT_EQUAL_STRING("12.4210", postProcess({0.0, 1.0, 1.9}, {4.3, 2.2, 1.0, 0.0}, a2dt).c_str());

Why do you want that hanging digit is getting corrected although analog value is already higher than threshold? As for my understanding, either digit arrrived too early at 1.9 which means it should be kept at 1 until analog also crosses zero or it's a hanging digit from last cycle then 'a2dt' threshold might be higher, because not such an early compensation would be needed. What's my mistake?

float a2dt = 3.6;
TEST_ASSERT_EQUAL_STRING("13.1210", postProcess({0.0, 1.0, 1.9}, {1.2, 2.2, 1.0, 0.0}, a2dt).c_str());

Why is the expected result 13.x and not 12.x?

---

float a2dt = 3.6;
// meter shows 011.0210 but it already needs to be 012.0210,  before transition
TEST_ASSERT_EQUAL_STRING("12.0210", postProcess({0.0, 1.0, 1.0}, {0.2, 2.2, 1.0, 0.0}, a2dt).c_str());

Why is the expected result 12.x instead 11.x?

Also some of the following in same function I do not understand. It seems your meter needs to be read 1 higher all the time versus what it's states or I'm totally wrong!? Sorry for all those questions...

[rainman110]

Dear @Slider0007 . First thank you for a great in depth code review. In fact, I had the impression, that my previous PR was merged too early without a proper code review.

Let my try to answer your questions:

Are you still using results from 'old' sync logic, meaning there are now two logics which are doing sequenceally almost the same?

Yes this is true and unfortunate. In fact, since I am new to the project, I could not completely understand the old logic and I started implementing my own - for myself and my problem of late transitioning, which was not addressed before. I would rather favour, if we can combine the old and new code.

How do differenacte that you accidentally not adjust twice?

The old logic is practically disabled in line

AI-on-the-edge-device/code/components/jomjol_flowcontroll/ClassFlowPostProcessing.cpp

Line 937 in b31a613

std::string digitValues = flowDigit->getReadout(j, true, previous_value, NUMBERS[j]->analog_roi->ROI[0]->result_float, 0.);

by using 0. as the last argument. So only the new sync algo is used.

Wouldn't it be better to do it either the one way or the other way do avoid confussion? I'm not the judge ;-)

Yes! And you obviously can judge this. I did not want to change to much old code - since I could not completely understand it. I would be happier, if we just had one algo. As I said, I hoped at that this was addressed at the first PR, when this was merged without a review (see #2778)

It seems there are now quite different understanding between early and late

Lets look at "my actual issue" back then: #2743

This is what I understand as late transitioning. In this case, the last digit transitions too late, i.e. when the analog value is at about ~3.5. This means, when the analog value is in the range of 0 ... 3.5, the last digit 1 too small. It's basically the inverse of early transition. Support for late transition was IMHO not implemented before this PR.

How do I differentiate between early and late? Late means, that the digit transitions between analog values of 0.5 ... 5. Early transition means, that the digit transitions between analog value 5 ... 9.5. Normal transition would be 9.5 ... 0.5.

In fact, my new code seems to be using different values than the old... which is not intentional. Please let me know, how we can improve the situation.

I do not understand some (most) of your new test cases. Maybe I did not get your interpretation of 'a2dt' correctly...

a2dt refers to the analogDigitialTransitionStart. In normal test cases, it should be 9.2. For late transitions, this value is between 0.5 and 5.

Why do you want that hanging digit is getting corrected although analog value is already higher than threshold?

Normally , nothing need to be done, when the value is higher than threshold. But in this test case, the last digit still hangs at 1.9 (so 1 would be used). After transition, it should be 2.0 though. Hence it needs to be corrected. Don't ask me, how often I needed to wrap my head around this. This problem seems to be so easy on first sight, but the more I think about it, the weirder it gets 😆 . I derived these test cases from my own device, which has these problems. So the tests should be correct ;)

Why is the expected result 13.x and not 12.x?

Excellent questions. In this case, we have a hanging digit and late transition. First lets assume the last digit would be not hanging, i.e. The first analog (1.2) is still smaller than the transition value (3.6). Ideally, the 2.0 would have been transitioned to 3.0 at analog = 0. Since we have late transitioning, the transition has not happened, so we need to correct for it to 3.0. Now we have a hanging digit of 1.9. So we have to correct twice, from 1 -> 2 (hanging) and from 2-> 3(late transition). I hope, the explanation is clear?

Why is the expected result 12.x instead 11.x?

The same explanation as above, but without hanging digit. The transition should have happened already, but did not due to the late transition issue.

How do we now proceed? Ideally, since you know the code much better than me, I'd prefer if we could use your? old code as a basis and modify it, to make my new unittests (*lateTransition) working. You can assume, that the tests are correct 😏

Let me know, how we proceed.

[rainman110]

@Slider0007 If I also might ask, how do you develop / debug the code? I have a not-so-nice setup to be able to cross-compile the code to PC and debug it. I hoped, there would be a way to simply build for esp32 and let the tests run in an emulator or so. Is there any way to do it?

[Slider0007]

@rainman110:
I'm a while with this project because it's quite interesting in different aspects, but in the past I did not really focus to different test cases. @haverland, @jomjol are way more experienced in test cases. I only ported the existing post-processing CNN getReadout logic completely to integer handling for some benefits and more code understanding.
Actually it's funny it was your test case which was letting me dig deeper into this area of software and the meaning of e.g. parameter 'AnalogDigitTransitionStart'. Here we are...

---

1. Lets look at "your actual issue" Allow even earlier analog to digital transition (AnalogDigitalTransitionStart < 6.0) #2743:

This is what I understand as late transitioning. In this case, the last digit transitions too late, i.e. when the analog value is at about ~3.5. This means, when the analog value is in the range of 0 ... 3.5, the last digit 1 too small. It's basically the inverse of early transition. Support for late transition was IMHO not implemented before this PR.

I think this is the point where you interprete it different or at least you name it differently :-)

At least for me the digit is almost reaching the 2 although analog is still quite low -> EARLY digit based on this interpretation https://jomjol.github.io/AI-on-the-edge-device-docs/Watermeter-specific-analog---digital-transition/.

My explanation: Your digit is getting quite early to region of 2. This early situation is not covered by firmware's 'old' logic, therefore after digit reaches the digit zero crossing uncertainty area (>= x.8 due to e.g. mechanical sync tolerances, alignment or model uncertainties), algo unfortunately prioritize 'hanging digits' handling (analog value <= AnalogDigitTransitionStart - which was limited to lowest of 6) over 'too early arrived digits' handling (analog value > AnalogDigitTransitionStart). Value gets corrected upwards and you got the jump in reading -> not desired. In your case 'do nothing' would be the expected reaction until analog also reaches zero crossing.

---

The old logic is practically disabled in line

AI-on-the-edge-device/code/components/jomjol_flowcontroll/ClassFlowPostProcessing.cpp

Line 937 in b31a613

std::string digitValues = flowDigit->getReadout(j, true, previous_value, NUMBERS[j]->analog_roi->ROI[0]->result_float, 0.);

by using 0. as the last argument. So only the new sync algo is used.

Ah ok, I didn't realized this small change during my quick check ;-) Unfortunately, AnalogDigitTransitionStart=0 does NOT turn off sync behaviour. It's still partly active.

---

3. Further test cases:

a2dt refers to the analogDigitialTransitionStart. In normal test cases, it should be 9.2. For late transitions, this value is between 0.5 and 5.

Why do you want that hanging digit is getting corrected although analog value is already higher than threshold?

Normally , nothing need to be done, when the value is higher than threshold. But in this test case, the last digit still hangs at 1.9 (so 1 would be used). After transition, it should be 2.0 though. Hence it needs to be corrected. Don't ask me, how often I needed to wrap my head around this. This problem seems to be so easy on first sight, but the more I think about it, the weirder it gets 😆 . I derived these test cases from my own device, which has these problems. So the tests should be correct ;)

For me it's the same case than your initial case which means keep value until analog has zero crossing.

Can you provide an screenshot for better understanding?

Why is the expected result 13.x and not 12.x?

Excellent questions. In this case, we have a hanging digit and late transition. First lets assume the last digit would be not hanging, i.e. The first analog (1.2) is still smaller than the transition value (3.6). Ideally, the 2.0 would have been transitioned to 3.0 at analog = 0. Since we have late transitioning, the transition has not happened, so we need to correct for it to 3.0. Now we have a hanging digit of 1.9. So we have to correct twice, from 1 -> 2 (hanging) and from 2-> 3(late transition). I hope, the explanation is clear?

Analog has zero crossing, digit is hanging -> therefore correct it upwards +1: 11.x --> 12.x

Can you provide an screenshot for better understanding?

---

If you are interested I can provide upfront a test version of my private copy with extended AnalogDigitTransitionStart range down to 3 and old logic supporting it for initial testing purpose (be aware: It's without any of your code and has sightly different REST / MQTT APIs). Your test case is already included. If this the right direction and I did it correctly we could continue this way.

@haverland, @jomjol: From my point of view it should be possible to add this early digit situation to existing algo as far as I can see this from my limited perspective. All other test cases still work. Porting back to this firmware should be feasible.

[Slider0007]

@Slider0007 If I also might ask, how do you develop / debug the code? I have a not-so-nice setup to be able to cross-compile the code to PC and debug it. I hoped, there would be a way to simply build for esp32 and let the tests run in an emulator or so. Is there any way to do it?

Sorry, I do not have any special environment in place, just a spare ESP32CAM board hooked up to PC.

[rainman110]

Here's a screenshot. Note, the digit 8 is hanging (7.9) and the transition has not started yet:

The correct value (in my current interpretation) is 419.2579.

And now to the interpretation. Sure, one can discuss,if this is also a case of very early transition. But what if the transition starts even earlier,let's say at 0.5 . Is this then extremely early or just a little late? Just handling everything as early has though the charme of just using one algorithm compared to two.

Porting back to this firmware shouldn't be feasible.

What do you mean? What cannot be ported back?

[Slider0007]

As preview, evaluation of late transition test cases with extended old logic:

void test_doFlowLateTransition()
{
        // in these test cases, the last digit before comma turns 3.6 too late
        float a2dt = 3.6;

        // meter shows 011.0210 but it already needs to be 012.0210,  before transition
        TEST_ASSERT_EQUAL_STRING("12.0210", postProcess({0.0, 1.0, 1.0}, {0.2, 2.2, 1.0, 0.0}, a2dt).c_str());

Result Algo: 11.0210
My opinion: OK, no correction neccessary --> Delta to your expected result unclear

        // meter shows 011.3210 but it already needs to be 012.3210, just before transition
        TEST_ASSERT_EQUAL_STRING("12.3210", postProcess({0.0, 1.0, 1.2}, {3.3, 2.2, 1.0, 0.0}, a2dt).c_str());

Result Algo: 11.3210
My opinion: OK, no correction neccessary --> Delta to your expected result unclear

        // meter shows 012.4210 , this is after transition
        TEST_ASSERT_EQUAL_STRING("12.4210", postProcess({0.0, 1.0, 2.0}, {4.3, 2.2, 1.0, 0.0}, a2dt).c_str());

Result Algo: 12.4210
My opinion: OK, early digit handling, so keep value until analog zero crossing

        // meter shows 012.987
        TEST_ASSERT_EQUAL_STRING("12.9870", postProcess({0.0, 1.0, 2.0}, {9.8, 8.7, 7.0, 0.0}, a2dt).c_str());

Result Algo: 12.9870
My opinion: OK, still early digit handling, so keep value until analog zero crossing

        // meter shows 0012.003
        TEST_ASSERT_EQUAL_STRING("13.003", postProcess({0.0, 0.0, 1.0, 2.0}, {0.1, 0.3, 3.1}, a2dt).c_str());

Result Algo: 12.003
My opinion: OK, no correction --> Delta to your expected result unclear

        // meter shows 0012.351
        TEST_ASSERT_EQUAL_STRING("13.351", postProcess({0.0, 0.0, 1.0, 2.8}, {3.5, 5.2, 1.1}, a2dt).c_str());

Result Algo: 13.351
My opinion: OK, correct hanging digit + 1

        // meter shows 0013.421
        TEST_ASSERT_EQUAL_STRING("13.421", postProcess({0.0, 0.0, 1.0, 3.0}, {4.1, 2.2, 1.1}, a2dt).c_str());
}

Result Algo: 13.421
My opinion: OK, early digit handling, so keep value until analog zero crossing

Three of them differs from your expectation.

[Slider0007]

Thanks for the screenshot.

The correct value (in my current interpretation) is 419.2579.

Here I do not understand why you add 1 even it's so close at 8 and far away from 9. It's just hanging a bit below 8, so therefore it's correcting the hanging digit from 7.9 to 8 because analog has already zero crossing.

Update: I added it also to test cases, for me it looks valid (wrong would be 417.x):

Porting back to this firmware shouldn't be feasible.
What do you mean? What cannot be ported back?

Sorry I wrote it wrong: Porting back from my fork to this firmware should be feasible.

And now to the interpretation. Sure, one can discuss,if this is also a case of very early transition. But what if the transition starts even earlier,let's say at 0.5 . Is this then extremely early or just a little late? Just handling everything as early has though the charme of just using one algorithm compared to two.

The transition start in my opinion does not really matter, it only matters when the digit comes into the area which is ambigious, in this case close before zero crossing, because it could be hanging from last cycle or it's to early of this cycle.

[rainman110]

To understand the test cases. These tests simulate are real 1000 liter Simulation. So it just starts shortly after 12 and ends at 13. All tests are in the correct sequence as they appear on my device. Hence, all resulting numbers need to be in ascending order. Whether they are all 1000 liters less, I don't care. But they obviously should not oscillate.

If you implement using too early transition, all digits need to be corrected -1, after the analog values is larger than 3.6 basically.

[rainman110]

Here's a screenshot at the actual transition.

As you can see, the digit transition is at a strange position, which makes it hard to read the correct value even manually.

[rainman110]

Here I do not understand why you add 1 even it's so close at 8 and far away from 9. It's just hanging a bit below 8, so therefore it's correcting the hanging digit from 7.9 to 8 because analog has already zero crossing.

If the analog value is 200 liter more, the digit had transitioned and it would show 419,4 right? So it must have been 419,2 (and not 418,2) earlier, otherwise 1200 liters would have flown.

[Slider0007]

To understand the test cases. These tests simulate are real 1000 liter Simulation. So it just starts shortly after 12 and ends at 13. All tests are in the correct sequence as they appear on my device. Hence, all resulting numbers need to be in ascending order. Whether they are all 1000 liters less, I don't care. But they obviously should not oscillate.

The only one which is not in ascending order is this one:

TEST_ASSERT_EQUAL_STRING("13.003", postProcess({0.0, 0.0, 1.0, 2.0}, {0.1, 0.3, 3.1}, a2dt).c_str());

Are you sure that the lowest digit is sure 2.0 and not maybe 2.8 - 3.x? {0.0, 0.0, 1.0, **2.0**}

If you implement using too early transition, all digits need to be corrected -1, after the analog values is larger than 3.6 basically.

Just to make it clear, this is the actual implementation right now, before your change, only limited to not lower than 6.0 because jomjol or haverland haven't forseen such an early entry. I more or less just lowered the value to 3 and did not implement anything else for detection.

[Slider0007]

Here I do not understand why you add 1 even it's so close at 8 and far away from 9. It's just hanging a bit below 8, so therefore it's correcting the hanging digit from 7.9 to 8 because analog has already zero crossing.

If the analog value is 200 liter more, the digit had transitioned and it would show 419,4 right? So it must have been 419,2 (and not 418,2) earlier, otherwise 1200 liters would have flown.

No, 200 liter are 200 liter :-) Your digit transition started but it's still 418.x, in this case 418.4x. Switch to real 419 can only happen if analog has next zero crossing.

[rainman110]

The only one which is not in ascending order is this one:

Your values are wrong, since they cover more than 2000 liters, but the simulation is just over 1000 liters. The jump from 11.3 to 12.4 is wrong. It was actually just 100 liters in between, but the digit changed from 1 to 2 in these 100 liters.

[rainman110]

I try to provide you a full real 1000 liter cycle of my device. It is wierder than you think ;)

[rainman110]

Switch to 419 can only happen if analog has next zero crossing.

For the actual value yes! The device no.

So either you interpret both value as 11.3 and 11.4 ,or 12.3 and 12.4. your tests though give 11.3 and then 12.4

[Slider0007]

The only one which is not in ascending order is this one:

Your values are wrong, since they cover more than 2000 liters, but the simulation is just over 1000 liters. The jump from 11.3 to 12.4 is wrong. It was actually just 100 liters in between, but the digit changed from 1 to 2 in these 100 liters.

OK, could be. Just copied your test cases as is.

Switch to 419 can only happen if analog has next zero crossing.

For the actual value yes! The device no.

So either you interpret both value as 11.3 and 11.4 ,or 12.3 and 12.4. your tests though give 11.3 and then 12.4

That's the funny thing. With test cases you can get everything you want if you'd like to, just by trimming to the corect input values. But I think that's not the goal ;-)

I try to provide you a full real 1000 liter cycle of my device. It is wierder than you think ;)

What kind of meter do you have!? ;-)

To makes it easier for me I try to prepare I test version with AnanlogDigitTransitionStart lowered to 3 from a commit before your code got merged. So you could test and compare by yourself.

Have a good night. I'm out for today...

[rainman110]

I think my meter is simply assembled incorrectly.

Nevermind. Thanks for the discussion.

Let's discuss it further in the next days.

[Slider0007]

As preview, evaluation of late transition test cases with extended old logic:

void test_doFlowLateTransition()
{
        // in these test cases, the last digit before comma turns 3.6 too late
        float a2dt = 3.6;

        // meter shows 011.0210 but it already needs to be 012.0210,  before transition
        TEST_ASSERT_EQUAL_STRING("12.0210", postProcess({0.0, 1.0, 1.0}, {0.2, 2.2, 1.0, 0.0}, a2dt).c_str());

Result Algo: 11.0210 My opinion: OK, no correction neccessary --> Delta to your expected result unclear

        // meter shows 011.3210 but it already needs to be 012.3210, just before transition
        TEST_ASSERT_EQUAL_STRING("12.3210", postProcess({0.0, 1.0, 1.2}, {3.3, 2.2, 1.0, 0.0}, a2dt).c_str());

Result Algo: 11.3210 My opinion: OK, no correction neccessary --> Delta to your expected result unclear

        // meter shows 012.4210 , this is after transition
        TEST_ASSERT_EQUAL_STRING("12.4210", postProcess({0.0, 1.0, 2.0}, {4.3, 2.2, 1.0, 0.0}, a2dt).c_str());

Result Algo: 12.4210 My opinion: OK, early digit handling, so keep value until analog zero crossing

        // meter shows 012.987
        TEST_ASSERT_EQUAL_STRING("12.9870", postProcess({0.0, 1.0, 2.0}, {9.8, 8.7, 7.0, 0.0}, a2dt).c_str());

Result Algo: 12.9870 My opinion: OK, still early digit handling, so keep value until analog zero crossing

        // meter shows 0012.003
        TEST_ASSERT_EQUAL_STRING("13.003", postProcess({0.0, 0.0, 1.0, 2.0}, {0.1, 0.3, 3.1}, a2dt).c_str());

Result Algo: 12.003 My opinion: OK, no correction --> Delta to your expected result unclear

        // meter shows 0012.351
        TEST_ASSERT_EQUAL_STRING("13.351", postProcess({0.0, 0.0, 1.0, 2.8}, {3.5, 5.2, 1.1}, a2dt).c_str());

Result Algo: 13.351 My opinion: OK, correct hanging digit + 1

        // meter shows 0013.421
        TEST_ASSERT_EQUAL_STRING("13.421", postProcess({0.0, 0.0, 1.0, 3.0}, {4.1, 2.2, 1.1}, a2dt).c_str());
}

Result Algo: 13.421 My opinion: OK, early digit handling, so keep value until analog zero crossing

Three of them differs from your expectation.

A last one for today: With slightly adapted values and a adaption of a2dt to 3.4 your ascending order is possible:

{0.0, 1.0, 1.0}, {0.2, 2.2, 1.0, 0.0}
--> I (10707) POSTPROC: default: Value: 11.0210, Rate per min: 0.00000, Status: 000 Valid

{0.0, 1.0, 1.2}, {3.3, 2.2, 1.0, 0.0}
--> I (10757) POSTPROC: default: Value: 11.3210, Rate per min: 0.00000, Status: 000 Valid

{0.0, 1.0, **1.9**}, {4.3, 2.2, 1.0, 0.0}
--> I (10807) POSTPROC: default: Value: 11.4210, Rate per min: 0.00000, Status: 000 Valid

{0.0, 1.0, **1.9**}, {9.8, 8.7, 7.0, 0.0}
--> I (10857) POSTPROC: default: Value: 11.9870, Rate per min: 0.00000, Status: 000 Valid

{0.0, 0.0, 1.0, 2.0}, {0.1, 0.3, 3.1}
--> I (10907) POSTPROC: default: Value: 12.003, Rate per min: 0.0000, Status: 000 Valid

{0.0, 0.0, 1.0, 2.8}, {3.5, 5.2, 1.1}
--> I (10967) POSTPROC: default: Value: 12.351, Rate per min: 0.0000, Status: 000 Valid

{0.0, 0.0, 1.0, **2.9**}, {4.1, 2.2, 1.1}
--> I (11017) POSTPROC: default: Value: 12.421, Rate per min: 0.0000, Status: 000 Valid

[Slider0007]

@rainman110:
Please find a branch with old logic and adapted AnalogDigitTransistionStart lowered from 6 to 3 to cover your early transition start for testing:
https://github.com/jomjol/AI-on-the-edge-device/tree/analog-digit-early-digit-test

Related firmware build for direct download: https://github.com/jomjol/AI-on-the-edge-device/actions/runs/7866238026

Be aware: It's not based on latest rolling, but on last commit before merge of your alternative logic. Also added by later merged test case fixes from haverland and tflite heap fix.

Goal: Figure out if this small change of the 'old' logic is sufficient to cover your meter which has a really early digit tranisition.

[rainman110]

Thank you..I am gonna try this out.

Should we still merge this PR as a quick fix to close the issue for now and help them with problems regarding the decimal shift?

[Slider0007]

Should we still merge this PR as a quick fix to close the issue for now and help them with problems regarding the decimal shift?

In the meantime there are also some issues described with too low raw value where decimal shift is not set or set to 0!? Will this then be also fixed or just the situations where decimal shift is set !=0?

[rainman110]

@FrankCGN01 The digit classification does not work for your last digit, probably due to the line through the digits. Maybe try a different model. Otherwise, add these images to the training set.

[FrankCGN01]

@rainman110 Some of the numbers with the line are already included in the known numbers. But the 8 with the dash is still missing. I will save the pictures in the next few days and then the missing numbers with the lines can be added to the model.

[rainman110]

@Slider0007 I suggest removing the failing test from @FrankCGN01 , since the inferred values are incorrect. This needs a model improvement instead.

[Slider0007]

// THIS TEST FAILS
//
// reported by penapena
// Note: this is a strange example, as the last digit (4.4) seems to have very early transition
//
// @todo: The analog to digital value needs to be determined. Feed required by @penapena
decimalShift = 0;
TEST_ASSERT_EQUAL_STRING("124.4981", postProcess({ 0.0, 1.0, 2.0, 4.4}, { 5.1, 9.8, 8.3, 1.6},
a2dt, decimalShift).c_str());

For me the example looks valid.
With 'old' logic succesfully processed, no sync correction would be in action. Just tested it.
Why is this failing in your logic? What can we do that this will work again? Is it possible that AnalogDigitTranisitionStart parameter is differently used in your logic and needs to be lower than before?

Update:

// THIS TEST FAILS
// THIS TEST CASE COULD BE INVALID
//
// Second test in #2857 (comment)
// The last digit seems to be falsely recongnized. It looks like a regular "2" (no transition)
// but it is recognized by the inference as "2.5".
//
// @todo: Feedback required by @FrankCGN01
decimalShift = -3;
TEST_ASSERT_EQUAL_STRING("159.5022", postProcess({ 0.9, 4.9, 9.0, 5.0, 0.0, 2.5}, { 2.2},
a2dt, decimalShift).c_str());

Also this one is processed successfully with 'old logic'!
Why is it failing with yours?

Some special test cases seems to be handled better by your logic, but it fails in some basic real examples. There seems still lots of open topics...

As long it's not clear why even those basic examples are not working anymore, we should in first step provide a stable firmware version. Even old logic is not fully cover all test edge cases.

Therefore we decided to revert your logic to previous logic to have stable version again. Sorry!
Long term your polished and fixed logic could be implemented again. But this needs some time...

[penapena]

@Slider0007 Most of the reported problems have been fixed.

There are still 2 peculiar new tests though, that are strange are require feedback. Please have a look at the comments in the new tests. IMHO, one test is invalid. The other test from @penapena is strange, since the analog value seems to behave irregular. Maybe a case of late or early transition. More feedback is required from @penapena and @FrankCGN01.

With this version (Development-Branch: HEAD (Commit: f4c4498+)) it works as it should at least for now.

[caco3]

For now I reverted the previous PR (#2778) with #2899.

This way we should be back on a stable version.
This gives us time to work on a more mature improvement which does not break the current implementation.

[rainman110]

@caco3 and @Slider0007 I am still working on a more improved sync, but I think it will take same time. I could fix some of the failing test though already.

I now stumbled upon one particular test, where I am not sure, which should be the correct expected value:

Digits: 0.9, 4.8, 9.0, 3.0, 6.0, 5.0
Analogs: 9.6

My new algorithm returns 149364.9.
The old algorithm reports 149365.9.

In theory, with a normal meter, the last digit should be in in the range of 4.5 - 5.0, when the analog almost is at the top at zero. Otherwise, the meter would transition too late. What do you think is the correct value in your interpretations?

@haverland What do you think?

[Slider0007]

@caco3 and @Slider0007 I am still working on a more improved sync, but I think it will take same time. I could fix some of the failing test though already.

I now stumbled upon one particular test, where I am not sure, which should be the correct expected value:

Digits: 0.9, 4.8, 9.0, 3.0, 6.0, 5.0 Analogs: 9.6

My new algorithm returns 149364.9.
The old algorithm reports 149365.9.

In theory, with a normal meter, the last digit should be in in the range of 4.5 - 5.0, when the analog almost is at the top at zero. Otherwise, the meter would transitio too late. What do you think is the correct value in your interpretations?

I think we should wait until @haverland also joins this discussioin. He has more history than me in this topic.

[haverland]

@caco3 and @Slider0007 I am still working on a more improved sync, but I think it will take same time. I could fix some of the failing test though already.

I now stumbled upon one particular test, where I am not sure, which should be the correct expected value:

Digits: 0.9, 4.8, 9.0, 3.0, 6.0, 5.0 Analogs: 9.6

My new algorithm returns 149364.9. The old algorithm reports 149365.9.

In theory, with a normal meter, the last digit should be in in the range of 4.5 - 5.0, when the analog almost is at the top at zero. Otherwise, the meter would transition too late. What do you think is the correct value in your interpretations?

@haverland What do you think?

Sorry for the late reply.
Is it part of a test case?
Result should be 5, because most digits starts the transition after 9.2 of the analog or digit before.

Look at this test case. Maybe the same:

AI-on-the-edge-device/code/test/components/jomjol-flowcontroll/test_flowpostprocessing.cpp

Line 357 in d6a1838

// Fehler bei V12.0.1

And look at the diskussion about the test case.
#1110 (comment)

[haverland]

I've added a complete transition in function test_flowpostprocessing.cpp#test_doFlowPP_rainman110_transition().

@Slider0007 @rainman110 could you review the input and expected results?

If it's ok for you, I can approve the pull request

[Slider0007]

I've added a complete transition in function test_flowpostprocessing.cpp#test_doFlowPP_rainman110_transition().

@Slider0007 @rainman110 could you review the input and expected results?

If it's ok for you, I can approve the pull request

@haverland:

I was wondering where you added the changes because the related branch of this PR is unchanged. Then I realized you modified the following test branch: https://github.com/jomjol/AI-on-the-edge-device/tree/analog-digit-early-digit-test

1. Be aware this test branch mentioned above is not linked with this PR. This PR was prepared by rainman110 to fix the algorithm which was part in 15.5 and which is already reverted since 15.6.
2. The branch https://github.com/jomjol/AI-on-the-edge-device/tree/analog-digit-early-digit-test was prepared by myself the same time based on the discussion here but based on existing algorithm instead of new rainman110 algo. If we'd like to merge the analog-digit-early-digit-test branch, firstly it needs to be rebased to latest rolling because right now it's quite outdated.
3. I've checked your added test cases. For me they all look valid.
4. Additionaly, I added one more test case which was raised from rainman110 in this thread, here (Fixed problems when decimal shifts != 0 #2887 (comment))
   
   Adding this test case, our algo fails on a digit to digit transition (159xxx.x instead 149xxx.x) By adapting one parameter the test case is running through. One older test case has to be sightly adapted as well, though. Please check the lastest commit in branch https://github.com/jomjol/AI-on-the-edge-device/tree/analog-digit-early-digit-test.

All test cases would be green with this configuration. Any concerns?

Be aware: With all the modifications the result of this new test case using our algo will be 149364.9 which is not aligned with your argumentation here: #2887 (comment)

[haverland]

Sorry for the confusion. I hadn't realized that the branch https://github.com/jomjol/AI-on-the-edge-device/tree/analog-digit-early-digit-test is not connected to the PR.

I first merged both branches from the current rolling.

But the rainman110:fix_issue_2857 showed more errors, also in the rainman110 test cases.

Therefore, I first added the additional test cases from rainman110:fix_issue_2857 to the analog-digit-early-digit-test.

Now the analog-digit-early-digit-test also has 4 test cases that fail.

[Slider0007]

@haverland: No problem :-) Thanks for merging latest rolling.

I've just checked the 4 failing cases of branch https://github.com/jomjol/AI-on-the-edge-device/tree/analog-digit-early-digit-test. --> I commited my proposed changes for discussion. All tests are green with these changes.

Please first check the remarks here.

---

1. test_doFlowLateTransition:FAIL: Expected '12.0210' Was '11.0210'
   // Slider0007: In my opinion this series starts clearly with 11.x.
   // As I remember right, this is a real series from rainman110, therefore the following cases
   // also needs to be corrected the same way
   --> After change, test cases: OK

TBD: It is a matter of interpretation, is it too late or is it quite early? Actual algo interprets as quite early and corrects value back.
BTW: With this meter even as human it's not easy to read correct value.

void test_doFlowLateTransition()
{
        // in these test cases, the last digit before comma turns 3.6 too late
        float a2dt = 3.4; // value when last digit reaches x.8 region

        // Questionable? (Meter shows 011.0210 but it already needs to be 012.0210,  before transition)
        // Slider0007: In my opionion this series starts clearly with 11.x
        // As I remember right, this is a real series from rainman110, therefore the following cases 
        // also needs to be corrected the same way
        TEST_ASSERT_EQUAL_STRING("11.0210", postProcess({0.0, 1.0, 1.0}, {0.2, 2.2, 1.0, 0.0}, a2dt).c_str());

        // meter shows 011.3210 but it already needs to be 012.3210, just before transition
        TEST_ASSERT_EQUAL_STRING("11.3210", postProcess({0.0, 1.0, 1.2}, {3.3, 2.2, 1.0, 0.0}, a2dt).c_str());

        // meter shows 012.4210 , this is after transition
        TEST_ASSERT_EQUAL_STRING("11.4210", postProcess({0.0, 1.0, 2.0}, {4.3, 2.2, 1.0, 0.0}, a2dt).c_str());

        // meter shows 012.987
        TEST_ASSERT_EQUAL_STRING("11.9870", postProcess({0.0, 1.0, 2.0}, {9.8, 8.7, 7.0, 0.0}, a2dt).c_str());

        // meter shows 0012.003
        TEST_ASSERT_EQUAL_STRING("12.003", postProcess({0.0, 0.0, 1.0, 2.0}, {0.1, 0.3, 3.1}, a2dt).c_str());

        // meter shows 0012.351
        TEST_ASSERT_EQUAL_STRING("12.351", postProcess({0.0, 0.0, 1.0, 2.8}, {3.5, 5.2, 1.1}, a2dt).c_str());

        // meter shows 0013.421
        TEST_ASSERT_EQUAL_STRING("12.421", postProcess({0.0, 0.0, 1.0, 3.0}, {4.1, 2.2, 1.1}, a2dt).c_str());
}

---

2. test_doFlowLateTransitionHanging:FAIL: Expected '12.4210' Was '11.4210'
   Same explaination than 1. Series starts with 11.x if we treat the readings as quite early.

If it should be valid hanging scenario my proposal is not valid. Algo is most likely not covering this, either correcting hanging or early transition, but not both. How do you interprete this?
BTW: With this meter even as human it's not easy to read correct value. Therefore for me it does make any difference if it late or early as long it's consistent handled always the the same way.

void test_doFlowLateTransitionHanging()
{
    float a2dt = 3.6;

    // Questionable? (meter shows 012.4210 , this is after transition)
    // Slider0007: In my opionion this series starts with 11.x with this a2dt value
    // As I remember right, this is a real series from rainman110, therefore the following cases 
    // also needs to be corrected the same way
    TEST_ASSERT_EQUAL_STRING("11.4210", postProcess({0.0, 1.0, 1.9}, {4.3, 2.2, 1.0, 0.0}, a2dt).c_str());

    TEST_ASSERT_EQUAL_STRING("11.6210", postProcess({0.0, 1.0, 1.9}, {6.3, 2.2, 1.0, 0.0}, a2dt).c_str());

    TEST_ASSERT_EQUAL_STRING("12.1210", postProcess({0.0, 1.0, 1.9}, {1.2, 2.2, 1.0, 0.0}, a2dt).c_str());

    TEST_ASSERT_EQUAL_STRING("12.3610", postProcess({0.0, 1.0, 2.5}, {3.5, 6.2, 1.0, 0.0}, a2dt).c_str());

    TEST_ASSERT_EQUAL_STRING("12.4510", postProcess({0.0, 1.0, 3.0}, {4.5, 5.2, 1.0, 0.0}, a2dt).c_str());

    TEST_ASSERT_EQUAL_STRING("12.4510", postProcess({0.0, 1.0, 2.9}, {4.5, 5.2, 1.0, 0.0}, a2dt).c_str());
}

---

3. test_doFlowEarlyTransitionEdgeCase:FAIL: Expected '99.50' Was '9999.50'
   Silder0007: In my opinion this is an unrealistic case {0.0, 0.0, 9.9, 9,0}, {5.0, 0.0} with this expected result --> More realistic values: {0.0, 0.9, 9.9, 9,0}, {5.0, 0.0}
   --> After change, test cases: OK

void test_doFlowEarlyTransitionEdgeCase()
{
    float a2dt = 8.;

    // Silder0007: In my opinion this is an unrealistic case {0.0, **0.0**, 9.9, 9,0}, {5.0, 0.0}
    // More realistic values: {0.0, 0.9, 9.9, 9,0}, {5.0, 0.0}
    TEST_ASSERT_EQUAL_STRING("99.50", postProcess({0.0, 0.9, 9.9, 9.0}, {5.0, 0.0}, a2dt).c_str());

    TEST_ASSERT_EQUAL_STRING("99.95", postProcess({0.0, 1.0, 0.0, 0.0}, {9.5, 5.0}, a2dt).c_str());
}

---

4. test_doFlowIssue2857:FAIL: Expected '159.3659' Was '149.3649'

• Topic: 159.x vs. 149.x:
  This is the only a little tricky test case, because it's quite on the edge of actual config. It really depends on the single decimal place. If this needs to be the one or other result, this would only be possible if we make the digit to digit transition also configurable the same way than analog/digit transition.

{ 0.9, 4.8, 8.9, 3.0, ... -> 1 5 9
{ 0.9, 4.8, 9.0, 3.0, ... -> 1 4 9

What is your opinion?

• Topic Analog/Digit Transistion: x.36 **4** 9 vs. x.36 **5** 9
  Adapt a2dt to 9.7 if result should be x.36 5 9
  Keep a2dt at 9.0 if result should be x.36 4 9
  This only depends on expectation. In my opinion both would be correct.
  What is your opinion?

void test_doFlowIssue2857()
{
  ...
    // FrankCGN01
    decimalShift = -3;
    a2dt = 9.7;
    TEST_ASSERT_EQUAL_STRING("159.3659", postProcess({ 0.9, 4.8, 8.9, 3.0, 6.0, 5.0}, { 9.6},
                                                    a2dt, decimalShift).c_str());

    // Second test in https://github.com/jomjol/AI-on-the-edge-device/issues/2857#issuecomment-1937452352
    // The last digit seems to be falsely recongnized. It looks like a regular "2" (no transition)
    // but it is recognized by the inference as "2.5".
    decimalShift = -3;
    TEST_ASSERT_EQUAL_STRING("159.5022", postProcess({ 0.9, 4.9, 8.9, 5.0, 0.0, 2.5}, { 2.2},
                                                      a2dt, decimalShift).c_str());
   ...

[haverland]

1.) No! Please not. Is the standard transition if digit and analog pointer transition fits. Means on 0.0 on analog pointer the digit is x.0. And a bit later the analog pointer is 0.2 and the digit x.0 until next transition after 9.2 on analog.

2.) I've added a case (or moved it up a few lines) for an analog pointer that transition is a bit earlier than the digit transition. Also often case.
Here is the issue for the extrem late transition. You have shortly a 12 and after 2.0 you got a 11 until the transition ends. This make the negatives. May we could optionaly do not except the small late transition. But who of the normal users understand such things and set the option correctly?

{
    float a2dt = 3.6;

    // haverland: this is the case if the analog pointer is a bit before the digit.
    // It's the normal late transition up to 2.0 on analog must the digit transition ends
    // After 2.0 on analog it named "hanging digit" by me. It never reach the x.0 until the next
    // transition begins.
    // BUT. It makes the issue you have later, because all other unter 3.6 are negative values now.
    TEST_ASSERT_EQUAL_STRING("12.1210", postProcess({0.0, 1.0, 1.9}, {1.2, 2.2, 1.0, 0.0}, a2dt).c_str());

3.) Shows an issue on every full digit transition. I've added the case with 1.0 (2nd test) it reduces the first digit. And creates shortly a real big negative rate or in case of 0099.5 a extremly high value. All after are now negatives

void test_doFlowEarlyTransitionEdgeCase()
{
    float a2dt = 8.;

    // Silder0007: In my opinion this is a unrealistic case {0.0, **0.0**, 9.9, 9,0}, {5.0, 0.0}
    // More realistic values: {0.0, 0.9, 9.9, 9,0}, {5.0, 0.0}
    TEST_ASSERT_EQUAL_STRING("99.50", postProcess({0.0, 0.0, 9.9, 9.0}, {5.0, 0.0}, a2dt).c_str());

    // fails with 99.50    
    TEST_ASSERT_EQUAL_STRING("199.50", postProcess({0.0, 1.0, 9.9, 9.0}, {5.0, 0.0}, a2dt).c_str());

4.) is in my opinion the same issue like 3.)

[haverland]

3.)/4.) I make a new branch/PR it's another issue and can fixed separately.

[Slider0007]

@haverland: In this case I don't get it correctly. Please feel free to take over the topic and discard my proposal.

[haverland]

I took another look at cases 3 and 4.

3. is not realistic, because the higher digit will not have the transition before the lower digit. Or I haven't seen it. So a
   {0.0, 9.7, 9.9, 9,0} or a { 0.0, 0.4, 9.7, 9.0 } is more realistic.

4. Has another issue in recognition. The second digit is a 5.1-5.2 not 4.9. It comes from incorrect ROI configuration. The ROIs are in height to high.
   https://jomjol.github.io/AI-on-the-edge-device-docs/ROI-Configuration/#setup-using-dig-class100-or-dig-cont-models describes the correct configuration.

But because of the rounding over x.8 it rounds up to 5. So the value is correct. The lowest digit has a false recognition because of the line in the 8. Could be better if the ROI configuration is correct.

[haverland]

But I'm struggling with case 1.) and 2.).

To solve it for the case of very early (or very late) transition, it would always break the standard. The short late transition algo will set it shortly to the next digit and after x.2 the "hanging" digit values reduce it again until the real transition is coming.