decouple hv charging from display update interval

[ThomasWaldmann]

  #define DISPLAYREFRESH 600000
  #define MAXCOUNTS 10000
  // Check if there are enough pulses detected or if enough time has elapsed.
  // If yes, then it is time to charge the HV capacitor and calculate the pulse rate.
  if ((GMC_counts >= MAXCOUNTS) || ((millis() - time2display) >= DISPLAYREFRESH )) {
    ...
    // charge HV capacitor
    ...
    // compute CPM
    ...
    // ... and display it.

The code is arranged a bit strangly - I guess it works ok as long as the DISPLAYREFRESH and MAXCOUNTS are kept about like they are, but I think logically, there are 2 issues here:

• The hv capacitor should be charged when needed and this has nothing to do with the DISPLAYREFRESH time.

• Looks like MAXCOUNTS is used for two purposes (so I guess we need MAXCOUNTRECHARGE and MAXCOUNTDISPLAY instead):

  • to trigger a display update (idea: if we have a high count, we can update more quickly without losing precision). How high that value should be for that purpose is a matter of precision requirement, see #38, so one might want to go to a higher value. (MAXCOUNTDISPLAY)
  • to trigger a hv capacitor recharge. this is independent from the other purpose and might need to be kept low to not let the capacitor discharge by too many gm pulses. (MAXCOUNTRECHARGE)

[ThomasWaldmann]

There is likely a similar issue with logging, which is also done inside that block (and thus coupled into that although it shouldn't be).

[rexfue]

A view lines above of the referred lines, there you see the charging of the cap. That happens every second. And that is independent of display or calculation time. So I think, we should delete the two lines in the display/calulation block, that charge the capacitor: HV_pulse_count = jb_HV_gen_charge__chargepules(); // charge HV capacitor hvpulsecnt2send += HV_pulse_count; // count for sending I'll do that in the next PR. Concerning logging: I guess it should be in that block: every time, we calc a new value, we log it to serial out, why not?

[ThomasWaldmann]

I didn't do voltage measurements, but I understood the code like that charging the hv capacitor is needed after some pulses because they discharge the capacitor more and more.

If the pulse rate is high, that once per second charge might not be enough maybe.

But that has nothing to do with the display update, that is my point.

Also, the logging should be outside of that block. Just imagine the case that the display would only update once ever 2 minutes. If you want to log more frequently, you can't do it in that every-2-minutes block. If you want to recharge, you can't do it in that block.

[rexfue]

On 18. Nov 2019, at 11:29, TW notifications@github.com wrote:

I didn't do voltage measurements, but I understood the code like that charging the hv capacitor is needed after some pulses because they discharge the capacitor more and more.

If the pulse rate is high, that once per second charge might not be enough maybe.

That has to be discussed with Jürgen. We can easily charge more often.

But that has nothing to do with the display update, that is my point.

Where do you see the dependency? Also, the logging should be outside of that block. Just imagine the case that the display would only update once ever 2 minutes. If you want to log more frequently, you can't do it in that every-2-minutes block. If you want to recharge, you can't do it in that block.

If we calculate only every 2 min, then we also can log only every 2 min. — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ecocurious/MultiGeiger/issues/66?email_source=notifications&email_token=AFSNFB5H5HWYD7NZTNDO3YTQUJVAPA5CNFSM4JOMDERKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEJ66JY#issuecomment-554954535, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFSNFB6D7A2J5UOHQXGE56TQUJVAPANCNFSM4JOMDERA.

[ThomasWaldmann]

The dependency in the current code comes from nesting unrelated stuff into the same if-block. Guess we need multiple if-conditions/blocks for different purposes.

Hmm, yeah, the 2min example was maybe not the best one. :-)

But there is a lot of different logging, so it should just depend on the calculation / availability of stuff to log, not necessarily on the display update. Just decouple stuff that should not be coupled.

[ThomasWaldmann]

note: guess for decoupling, we also need different GMC counters as we will want to reset them to 0 as soon as we "do something" when reaching some threshold or some other criteria:

• update display (currently: > GMC 100 counts or 10s elapsed)
• recharge hv (currently: when display update or when 1 minute elapsed)
• logging (when 150s elapsed, here we already have separate counters "...2send")

maybe instead of adding even more counters to the ISR, we could reduce there to 1 counter and maintain the other counters only in the main loop. that would make the critical sections shorter/quicker.

it could be also an opportunity to clarify variable names / make stuff more consistent and easier to understand.

[ThomasWaldmann]

removing this from "freiburg workshop" milestone as it is too close and this change has some risk to destabilize the code temporarily.

[ThomasWaldmann]

fixed by #247. display update is now decoupled from charging.