PaxInstruments / t400-firmware

Firmware for the Pax Instruments T400 temperature datalogger
22 stars 5 forks source link

Button delay #189

Closed charlespax closed 9 years ago

charlespax commented 9 years ago

It appears as if not all button presses are being executed properly. Particularly, the record button does not always work.

It does not seem to be a hardware issues. Hooked up a multimeter between SW_B and GND, each button press was registered by the multimeter.

I can make the record button press consistently register properly in the firmware. Press record, then press backlight, repeat. Each time the record button is pressed the recording will either start or stop as long as the backlight button is pressed between each record button press. This is also true for buttons that are disabled during recording.

In t400.ino we have,

if(Buttons::pending()) {
    uint8_t button = Buttons::getPending();
    ...

I think the issue is has something to do with the pending button queue.

charlespax commented 9 years ago

In buttons.cpp

void setup() {

  for(uint8_t b = 0; b < BUTTON_COUNT; b++) {
    if(activeState[b] == LOW) {
      pinMode(buttonPins[b], INPUT_PULLUP);
    }
  }

  // SW_A   Logging start/stop  INT3    PD3  // Incorrect. SW_B is PD3
  EICRA |= 0x40;    // Configure INT3 to trigger on any edge
  EIMSK |= _BV(INT3);    // and enable the INT3 interrupt

  // SW_B   Logging interval    INT6    PD2  // Incorrect. PD2 is INT2. SW_A is INT6 is PE6
  EICRB &= ~0x30;    // Configure INT6 to trigger on low level
  EIMSK |= _BV(INT6);    // and enable the INT6 interrupt

  // SW_C   Temperature units   PCINT4  PB4  // Correct
  // SW_D   Toggle channels     PCINT5  PB5  // Correct
  // SW_E   Backlight               PCINT6  PB6  // Correct
  // SW_PWR      Power on/off            PCINT7  PB7  // Correct
  PCMSK0 |= 0xF0;
  PCICR |= _BV(PCIE0);
}

PCB version 0.13 is screen shot 2015-10-06 at 13 26 29

It looks like maybe how the interrupts are configured based on the input type.

Input Is now Should be
SW_A PD3(INT3) PE6(INT6)
SW_B PD2(INT6) (sic)] PD3(INT3)
SW_C PB4(PCINT4) PB4(PCINT4)
SW_D PB5(PCINT5) PB5(PCINT5)
SW_E PB6(PCINT6) PB6(PCINT6)
SW_PWR PB7(PCINT7) PB7(PCINT7)
RTC_INT PD2(INT2)

I think the code should be

  // SW_A   Logging interval  INT6  PE6
  EICRB &= ~0x30;    // Configure INT6 to trigger on low level
  EIMSK |= _BV(INT6);    // and enable the INT6 interrupt

  // SW_B   Logging start/stop  INT3  PD3 on any edge
  EICRA |= 0x40;    // Configure INT3 to trigger on any edge
  EIMSK |= _BV(INT3);    // and enable the INT3 interrupt

  // SW_C   Temperature units   PCINT4  PB4
  // SW_D   Toggle channels     PCINT5  PB5
  // SW_E   Backlight               PCINT6  PB6
  // SW_PWR      Power on/off            PCINT7  PB7
  PCMSK0 |= 0xF0;
  PCICR |= _BV(PCIE0);
charlespax commented 9 years ago

I thought this might be a debounce issue, so I added small delay to the function that deals with PE6(INT6).

// button interrupts
ISR(INT6_vect) {
  // Workaround for the issue that ISR6 needs to be level sensitive to wake the processor from power down:
  // If we got here and the switch on INT6 is low (button pressed), switch to rising mode so we don't get stuck here
  // If we got here and the switch on INT6 was was high (button released), switch to level mode so we can wake the processor
  // In addition, don't put the processor in POWER_DOWN sleep mode when INT6 is level sensitive or the switch on INT6 won't
  // be able to wake the processor
  if(digitalRead(BUTTON_B_PIN) == LOW) {
    EICRB |= 0x30;    // Configure INT6 to trigger on rising edge
    delay(10);  // Added
    set_sleep_mode(SLEEP_MODE_IDLE);
  }
  else {
    EICRB &= ~0x30;    // Configure INT6 to trigger on low level
    delay(10);  // Added in
    set_sleep_mode(SLEEP_MODE_PWR_DOWN);
  }

  Buttons::buttonTask();
}

Now there seems to be no problem. I guess this was just a debouncing issue. I should put this on the scope.

charlespax commented 9 years ago

There are potentially some bounce issues, but I did find a configuration issue. In buttons.cpp line 106 should be button A rather than button B since button A is what's on INT6. This seems to work.

Should be:

if(digitalRead(BUTTON_A_PIN) == LOW) {
    EICRB |= 0x30;    // Configure INT6 to trigger on rising edge
    set_sleep_mode(SLEEP_MODE_IDLE);
  }

There is a debounce issue here. Some times a long press and release on will register on both press and release. There is also a large bounce when releasing the switch seen on the Oscope.

charlespax commented 9 years ago

It appears that determining the log file name is delaying things (https://github.com/PaxInstruments/t400-firmware/issues/190). Bouncing is another issue (https://github.com/PaxInstruments/t400-electronics/issues/212).

Tsillen commented 9 years ago

You can also just put a rc circuit on the switch input and leave the firmware as is.

charlespax commented 9 years ago

I'm looking into that now. Do you have specific values or configuration in mind?

Tsillen commented 9 years ago

You could also just test with the pull-up enabled. If that is not enough than you can try rc. Just calculate the time constant and determine how much you need.

http://www.engscope.com/pic-example-codes/basic-io-button-debounce/

charlespax commented 9 years ago

This is addressed in other issues now. Closing this one.