ole00 / afterburner

GAL chip programmer for Arduino
166 stars 46 forks source link

Some code questions #53

Closed hubertushirsch closed 8 months ago

hubertushirsch commented 9 months ago

I'm trying to understand your code better. I noticed sequences that were not clear to me.

afterburner.ino

0120: #define READPES 2

1033: // GAL init sequence
      static void turnOn(char mode) {
          setupGpios(OUTPUT);

          if (mode == READPES) {
              mode = 2;               // mode = 2 = READPES -> there is no change
1039:     } else
..........
1065:     setVPP(mode);
          delay(20);
1068: }

You test mode for the value READPES, which is defined with the value 2. if mode == 2 then mode = 2 ???

afterburner.ino There is a global variable vpp, which is set to a type-specific value depending on the GAL type. In setVpp() you calculate a value v (line 852), which is used in varVppSet(v) (line 860) as an index in vppWiper[v] . vppWiper[] contains the wiper values for the calibrated voltages from the EEPROM.

In lines 849/850 you set vpp = 40 for a limitation to 12V. But your formula leads to 10V (v = 2 (index VPP_10V0)). See my included comments. 12V would be the result at vpp = 48 ==> v = 6 (index VPP_12V0). Which is correct here, 10V or 12V?

0379: static uint8_t vpp = 0;

0832: static void setVPP(char on) {
          // new board desgin
          if (varVppExists) {
0835:         // uint8_t v = VPP_11V0;
              uint8_t v;     // v is recalculated in each case; initialization of the variable is unnecessary
..........
0844:         } else {
                  //safety check
                  if (vpp < 36) {      // set vpp < minimum to 9V0
                      vpp = 36; //9V   // ==> v = (36 / 2) - 18 = 0 ==> VPP_9V0
                  } else
0849:             if (vpp > 66) {      // set vpp > maximum to 10V0
0850:                 vpp = 40; //12V  // ==> v = (40 / 2) - 18 = 2 ==> VPP_10V0 !!!!!!
0851:             }
                  // 36 <= vpp <= 66      ==> v = 0 ... 15 ==> VPP_9V0 ... VPP_16V5
0852:             v = (vpp >> 1) - 18; // 18: 2 * 9V, resolution 0.5V (not 0.25V) hence 'vpp >> 1'
      #if 0                  
                  ......
      #endif                  
              }
0860:         varVppSet(on ? v : VPP_5V0);
              delay(50); //settle the voltage
0862:     }
ole00 commented 9 months ago

The first snippet (mode = 2): it is there for posterity, just to remind me when the VPP is turned on during PES reading, the mode is set to 2. It could be optimised a little bit, but the code works fine as it is.

The second question (Which is correct here, 10V or 12V?). I see what you mean, the comment is not correct. Yes, it would be worth setting the vpp to 12V. I'll do it eventually.

Thanks for the report.

hubertushirsch commented 9 months ago

Thanks for the explanation.

I was wondering because you are calling setVpp(mode) with mode (with the numeric values 0, 1 or 2) as an argument (line 1065). In setVpp() the argument is evaluated with if (on == READPES) (line 838). So in turnOn() READPES becomes the numeric 2 in order to be compared again with READPES in setVpp(). Very confusing.

Why are the arguments mode in turnOn() and on in setVpp() declared as char even though they are used with numeric values?

Code line numbers based on v0.5.6

0832: static void setVPP(char on) {
          // new board desgin
          if (varVppExists) {
              uint8_t v = VPP_11V0;

              // when PES is read the VPP is not determined via PES
0838:         if (on == READPES) {
0839:             if (gal == ATF16V8B || gal == ATF20V8B || gal == ATF22V10B || gal == ATF22V10B) {
      .....
0874: }

1034: static void turnOn(char mode) {
          setupGpios(OUTPUT);

          if (mode == READPES) {
              mode = 2;      
1039:     } else
      .....
1065:     setVPP(mode);
          delay(20);
1068: }

For example, using an enumeration for the three different values of mode could improve clarity.

typedef enum {
  VppOff,
  VppOn,
  VppReadPES
} VppMode;

Greetings

hubertushirsch commented 8 months ago

ok, I see it is corrected to 12V (vpp = 48).