Interrupt handler in library

Question

This is my first library so feel free to burn me and vote this down :D This is a library that provides me with Watchdog deep sleep on my ATtiny85 projects.

In the test sketch I use the watchdog timer's interrupt vector but I want to place this within the library. How can I do this?

I have tried to add this to the init method but that did not work out;
powernap.h:26: error: expected unqualified-id before string constant ISR(WDT_vect) {

Also; feel free to shoot at the rest of the library, I am here to learn.

Test Sketch

#include "powernap.h"

#define LED 0

Napper napper = Napper();

volatile boolean f_wdt = 1;

ISR(WDT_vect) {
  f_wdt=1;
}

void setup() {
  delay(2000);
  napper.setup(8); // configure watchdog timer
}

void loop() { // blink
  pinMode(LED, OUTPUT);
  digitalWrite(LED, LOW);
  delay(200);
  digitalWrite(LED, HIGH);
  delay(200);
  digitalWrite(LED, LOW);
  delay(200);

  napper.sleep(); // deep sleep
}

powernap.cpp

#include "powernap.h"

 #ifndef cbi
   #define cbi(sfr, bit) (_SFR_BYTE(sfr) &= ~_BV(bit))
 #endif
 #ifndef sbi
   #define sbi(sfr, bit) (_SFR_BYTE(sfr) |= _BV(bit))
 #endif

Napper::Napper() {

}

void Napper::init(float initial) {
//  volatile boolean f_wdt = 1;

//  ISR(WDT_vect) {
//  f_wdt=1;
//  }
}

void Napper::setup(int cycle) {
    byte bb;
    int ww;
    if (cycle > 9 ) cycle=9;
    bb=cycle & 7;
    if (cycle > 7) bb|= (1<<5);
    bb|= (1<<WDCE);
    ww=bb;

    MCUSR &= ~(1<<WDRF);
    // start timed sequence
    WDTCR |= (1<<WDCE) | (1<<WDE);
    // set new watchdog timeout value
    WDTCR = bb;
    WDTCR |= _BV(WDIE);
}

void Napper::sleep() {
    cbi(ADCSRA,ADEN);                    // switch Analog to Digitalconverter OFF

    set_sleep_mode(SLEEP_MODE_PWR_DOWN); // sleep mode is set here
    sleep_enable();

    sleep_mode();                        // System sleeps here

    sleep_disable();                     // System continues execution here when watchdog timed out
    sbi(ADCSRA,ADEN);                    // switch Analog to Digitalconverter ON
}

powernap.h

#ifndef Fader_h
#define Fader_h

#if ARDUINO >= 100
  #include "Arduino.h"
#else
  #include "WProgram.h"
  #include "pins_arduino.h"
  #include "WConstants.h"
#endif

#include <avr/sleep.h>
#include <avr/wdt.h>
#include <util/delay.h>

class Napper {
    public:
        Napper();

        void init(float initial);
        void setup(int cycle);
        void sleep();

    private:
        int flag;

};

#endif

Answer 1

To answer your actual question, you just have to define ISR(WDT_vect) outside any function.

Now, for the general critique, let me start by quoting Antoine de Saint Exupéry: “It seems that perfection is attained not when there is nothing more to add, but when there is nothing more to remove.” There are quite a few things to remove from your code, like unused variables, parameters or methods. I understand that you may be tempted to leave useless stuff in there anticipating that you may need it in a future, extended version. But please, don't do that. Try to keep clean and lean any code that you post in public, and only add extra stuff when you actually need it.

I would remove most of the #includes from powernap.h. If the compiler needs one of these to understand your .h, then live it there. If it's only needed to compile your library, then it belongs to your .cpp.

Also, there is no point in making your library dependent on the Arduino core, as you are using no core functionality. By removing this dependency, you can make your library more versatile, as it can be used both for Arduino and non-Arduino projects.

The line #ifndef Fader_h looks weird. Standard practice is to use the name of the .h file, capitalized. Don't depart from common practice unless you have some reason to do so.

I would also add static to your methods, this way they can be used without having to instantiate an object, like in Napper::setup(WDTO_4S);.

You are using three different syntaxes for setting and clearing bits in I/O registers. Your code will look more consistent if you choose one and stick with it.

I almost forgot... you have to make sure the timed sequence is done atomically, i.e. it's not interrupted.

Below is a version of the library that implements the suggestions above:

powernap.h

#ifndef POWERNAP_H
#define POWERNAP_H

#include <stdint.h>

class Napper {
    public:
        Napper() {}
        static void setup(uint8_t cycle);
        static void sleep();
};

#endif  // defined(POWERNAP_H)

powernap.cpp

#include <avr/interrupt.h>
#include <avr/sleep.h>
#include <util/atomic.h>
#include "powernap.h"

// This interrupt is used only as a wakeup source.
EMPTY_INTERRUPT(WDT_vect);

// Configure the watchdog as an interrupt source.
// `cycle' should be an integer from 0 to 9, or one of the constants
// WDTO_XXS from <avr/wdt.h>.
void Napper::setup(uint8_t cycle) {
    uint8_t prescaler;         // prescaler bits in WDTCR
    if (cycle > 9) cycle = 9;  // largest supported value
    prescaler = cycle & 7;     // bits WDP2:WDP0 are right aligned in WDTCR
    if (cycle > 7) prescaler |= _BV(WDP3);

    MCUSR &= ~_BV(WDRF);  // clear this bit: it overrides WDE
    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
        WDTCR |= _BV(WDCE)    // start timed sequence
              |  _BV(WDE);
        WDTCR =  prescaler    // set new watchdog timeout value
              |  _BV(WDIE);   // enable watchdog interrupt
    }
}

void Napper::sleep() {
    set_sleep_mode(SLEEP_MODE_PWR_DOWN);
    sleep_mode();
}

Answer 2

It is hard to help you improve your library or code if you don't tell others what your code pieces are trying to do.

In general, you will have a piece of code that sets up the whole thing, a piece of code that will be executed in the isr, and other helper code pieces, like getting status and clear status, as appropriate.

I often use a function pointer that the user can hook to the user handler so my isr portion of the code is fixed. Just hook to different functions in the user space and you are ready to go.

Obviously other ways exist, each with their pros and cons.