Porting Arduboy2 library to SAMD

Who is Martin L?

Can I suggest changing this to:

if (duration > 0)
{
	--duration;
	if(duration == 0)
	{
		REG_TC4_CTRLA &= ~TC_CTRLA_ENABLE; // TODO: check disable is recognised, may need to NAND with a bitmask
		while (TC4->COUNT16.STATUS.bit.SYNCBUSY);  
	}
}

I know you’re just copying what’s in the original,
but while there’s rewriting going on, might as well make a few improvements.

Is there a reference to what all these platform-specific things are somewhere?
(If SYNCBUSY isn’t a bool then I might have a few more suggestions.
Using non-bools in an if or while is one item in a long list of programming habits that I disapprove of for various reasons.)

I’m particularly interested in why it has to be TC4_Handler() specifically.
It seems there’s also a TC3, TC5, TC6 and TC7 (though TC5 is apparently already used by Arduino for Tone.cpp).

You shouldn’t need to use interrupts at all for the BeepPin classes. You just set the timer to generate the required square wave frequency and let the timer() function handle the duration, as my AVR version does. The SAMD21 counter/timers are capable of generating free running frequencies. Not having to deal with interrupts was one of the ideas of the BeepPin classes.

They’re mappings to the peripheral control registers as described in the datasheet. The mapping names match those used in the datasheet, with some abstraction.

He’s a guy on the Arduino forum who posted example TC4 code. I didn’t want to fail to attribute, so popped a comment in. In the rabbit hole of ArduboyTones I included the link in the comments but it seems not to have made it across to Arduboy2Beep. Can be changed if appropriate.

Noted.

It doesn’t but that’s what I have working at the moment. We should be able to use waveform out (W/O) on the pins. Currently we have Speaker1 on Arduino pin 5, assigned to PA15, which will take W/O from TC3 so I can try and work out how to configure that as a 50% duty square wave.

Unfortunately it doesn’t look like the pin we assigned to Speaker2 (PA02) will take a TC W/O.

But we chose that for its DAC so I presume we’re planning to be more sophisticated with audio from that in future. We can look for a different (currently unused) pin for Speaker2 that will take a W/O if we want?


EDIT: This may come in handy: http://shawnhymel.com/1710/arduino-zero-samd21-raw-pwm-using-cmsis/

Yes, is see you’re right. (And I did choose it for the DAC.)

I guess we’ll have to look at some other way to generate a 50% waveform on this pin, to support the BeepPin API.

For the BeepPin functions, and in general, I wanted to avoid using interrupts in the Arduboy2 library. (For AVR at least) all source for a library is compiled if you include it in your sketch. Any interrupt service routines (ISRs) in the library are then always linked into your sketch, regardless of whether they are used or not. I assume this is because it’s difficult or impossible to determine if an ISR will be used. This means that once you have an ISR in your library no other library can make use of that interrupt. (You get a compile error.)

This is why I removed the Playtune functionality, that was part of the original Arduboy library, and moved it to the separate ArduboyPlaytune library. Otherwise, other libraries (such as ArduboyTones) couldn’t make use of the same timer interrupt(s).

I haven’t researched it, so I don’t know if this interrupt restriction also applies to ARM based Arduino hardware. If it does, then it’s something that has to be kept in mind.

1 Like

Agree. I am optimistic as there is at least one tutorial on making a function generator using the DAC. But it could be reliant on interrupts!

Could you say whether the speaker 2 pin is used as a push pull mode (inverse logic level to speaker 1) for volume or whether the concept of operation is different? If push pull I couldn’t see any code to ensure they were out of phase when toggling in the AVR version but I could easily have missed it.

Not for the BeepPin functions. I intended BeepPin to provide two separate channels only. I provide control of each pin mainly so that the other pin could be used by another audio library at the same time (such as another library generating a sound track while BeepPin provides simple sound effects).

However, this will be a problem with porting ArduboyTones and other libraries that rely on this technique.

1 Like

To elaborate on this:
Arduino makes use of the ARM “Cortex Microcontroller Software Interface Standard (CMSIS)”. The CMSIS for a given microcontroller is provided in what’s known as a “CMSIS-Pack”.

The CMSIS-Packs for the Microchip/Atmel ARM microcontrollers can be found at:
http://packs.download.atmel.com/
(These are ZIP files.)

The files that abstract the peripheral control registers can be found in the include directories, specifically in the component and instance subdirectories.

For Arduino SAMD21, I found copies of these files at:


(They may be in other places as well. I haven’t looked.)


An example, for TC4->COUNT16.STATUS.bit.SYNCBUSY

Click to expand/collapse

TC4 is the base address for the Timer/Counter 4 control register block. It’s defined in the header file for the specific part. For example: for the SAMD21G18A it’s in file include/samd21g18a.h

#define TC4               ((Tc       *)0x42003000UL) /**< \brief (TC4) APB Base Address */

The timer counter control register block is mapped in file include/component/tc.h

COUNT16 is the register mapping for a counter/timer in 16 bit mode. It’s defined in union type Tc, which TC4 points to.

typedef union {
       TcCount8                  COUNT8;      /**< \brief Offset: 0x00 8-bit Counter Mode */
       TcCount16                 COUNT16;     /**< \brief Offset: 0x00 16-bit Counter Mode */
       TcCount32                 COUNT32;     /**< \brief Offset: 0x00 32-bit Counter Mode */
} Tc;

COUNT16 is a structure of type TcCount16

typedef struct { /* 16-bit Counter Mode */
  __IO TC_CTRLA_Type             CTRLA;       /**< \brief Offset: 0x00 (R/W 16) Control A */
  __IO TC_READREQ_Type           READREQ;     /**< \brief Offset: 0x02 (R/W 16) Read Request */
  __IO TC_CTRLBCLR_Type          CTRLBCLR;    /**< \brief Offset: 0x04 (R/W  8) Control B Clear */
  __IO TC_CTRLBSET_Type          CTRLBSET;    /**< \brief Offset: 0x05 (R/W  8) Control B Set */
  __IO TC_CTRLC_Type             CTRLC;       /**< \brief Offset: 0x06 (R/W  8) Control C */
       RoReg8                    Reserved1[0x1];
  __IO TC_DBGCTRL_Type           DBGCTRL;     /**< \brief Offset: 0x08 (R/W  8) Debug Control */
       RoReg8                    Reserved2[0x1];
  __IO TC_EVCTRL_Type            EVCTRL;      /**< \brief Offset: 0x0A (R/W 16) Event Control */
  __IO TC_INTENCLR_Type          INTENCLR;    /**< \brief Offset: 0x0C (R/W  8) Interrupt Enable Clear */
  __IO TC_INTENSET_Type          INTENSET;    /**< \brief Offset: 0x0D (R/W  8) Interrupt Enable Set */
  __IO TC_INTFLAG_Type           INTFLAG;     /**< \brief Offset: 0x0E (R/W  8) Interrupt Flag Status and Clear */
  __I  TC_STATUS_Type            STATUS;      /**< \brief Offset: 0x0F (R/   8) Status */
  __IO TC_COUNT16_COUNT_Type     COUNT;       /**< \brief Offset: 0x10 (R/W 16) COUNT16 Counter Value */
       RoReg8                    Reserved3[0x6];
  __IO TC_COUNT16_CC_Type        CC[2];       /**< \brief Offset: 0x18 (R/W 16) COUNT16 Compare/Capture */
} TcCount16;

STATUS is the status register, a member of COUNT16, mapped as union TC_STATUS_Type

typedef union {
  struct {
    uint8_t  :3;               /*!< bit:  0.. 2  Reserved                           */
    uint8_t  STOP:1;           /*!< bit:      3  Stop                               */
    uint8_t  SLAVE:1;          /*!< bit:      4  Slave                              */
    uint8_t  :2;               /*!< bit:  5.. 6  Reserved                           */
    uint8_t  SYNCBUSY:1;       /*!< bit:      7  Synchronization Busy               */
  } bit;                       /*!< Structure used for bit  access                  */
  uint8_t reg;                 /*!< Type      used for register access              */
} TC_STATUS_Type;

bit is a structure in STATUS containing bit fields for a uint8_t and SYNCBUSY is one of those bit fields, as seen above.

2 Likes

For generating a square wave on the DAC pin (PA02), you would use a timer and set the DAC between its lowest and highest value. But using the DAC for this isn’t necessary. You could just toggle the pin high and low in basic digital output mode.

Without a timer “waveform output” on PA02, I think you’re right that a timer using an interrupt would be required. Unlike AVR, the SAMD21 allows the interrupt vector table to be relocated into RAM space, which would allow changing interrupt vectors dynamically, but I don’t know if this is something that Arduino makes easy to facilitate and coordinate.

If it turns out to be difficult to share or reassign interrupts, given that no other audio libraries requiring interrupts have been ported yet, perhaps we could designate one timer (likely TC3 or TC4) to be “owned” by the Arduboy2 library. It would contain the timer’s ISR but that ISR would allow the real ISR to be set using an API.

For SAMD, the Arduboy2 library would have functions like:

bool assignTimerInterrupt(address)
bool releaseTimerInterrupt()

The address would be stored in a variable owned by the library. For “assign” the return status would indicate a failure if the interrupt was already assigned. For “release” the return status would indicate trying to release when not assigned. (I’m not sure that a “release” function would actually be necessary, since you can’t do the equivalent with AVR.)

The actual ISR would do nothing if no “destination” ISR was attached. Otherwise, the code at the given address would be executed like a regularly assigned ISR.

The BeepPin class for PA02 would assign the ISR in its begin() function. Other audio libraries would use the “assign” function in place of a direct ISR assignment.

One drawback of this approach is that libraries using it would be dependant on the Arduboy2 library, and libraries trying to use the interrupt directly with Arduboy2 in use would cause a conflict, but I don’t think that would be much of an issue, since the library system for the Arduboy is fairly tight-knit.

1 Like

I will also look into the events system. I recently got to use the events system on the ATtiny 0 series and it was a wonderful way of freeing the CPU with no need to service ISRs.

From a little more reading I understand that one of those (TC4, IIRC) is used for Arduino functions like millis() and micros(), although I’d need to double check as I also read that these functions use systick (or whatever that ARM feature is) and I don’t know whether they are the same thing or different!

I’ll hopefully have some progress to report on a Speaker 1 pulse generator that is based on W/O rather than interrupts in a short while. Stay tuned…

This compiles and works as a standalone interrupt-free. I’ll now integrate with Arduboy2Beep and let you know when I’ve tested and pushed the commit.

uint32_t period = 14*20  - 1;
 
void setup() {

  // Because we are using TCC0, limit period to 24 bits
  period = ( period < 0x00ffffff ) ? period : 0x00ffffff;

  // Enable and configure generic clock generator 4
  GCLK->GENCTRL.reg = GCLK_GENCTRL_IDC |          // Improve duty cycle
                      GCLK_GENCTRL_GENEN |        // Enable generic clock gen
                      GCLK_GENCTRL_SRC_DFLL48M |  // Select 48MHz as source
                      GCLK_GENCTRL_ID(4);         // Select GCLK4
  while (GCLK->STATUS.bit.SYNCBUSY);              // Wait for synchronization

  // Set clock divider of 1 to generic clock generator 4
  GCLK->GENDIV.reg = GCLK_GENDIV_DIV(1) |         // Divide 48 MHz by 1
                     GCLK_GENDIV_ID(4);           // Apply to GCLK4 4
  while (GCLK->STATUS.bit.SYNCBUSY);              // Wait for synchronization
  
  // Enable GCLK4 and connect it to TCC0 and TCC1
  GCLK->CLKCTRL.reg = GCLK_CLKCTRL_CLKEN |        // Enable generic clock
                      GCLK_CLKCTRL_GEN_GCLK4 |    // Select GCLK4
                      GCLK_CLKCTRL_ID_TCC0_TCC1;  // Feed GCLK4 to TCC0/1
  while (GCLK->STATUS.bit.SYNCBUSY);              // Wait for synchronization

  // Divide counter by 256 giving 187.5kHz (5.33 us) on each TCC0 tick
  TCC0->CTRLA.reg |= TCC_CTRLA_PRESCALER(TCC_CTRLA_PRESCALER_DIV256_Val);

  // Use "Normal PWM" (single-slope PWM): count up to PER, match on CC[n]
  TCC0->WAVE.reg = TCC_WAVE_WAVEGEN_NPWM;         // Select NPWM as waveform
  while (TCC0->SYNCBUSY.bit.WAVE);                // Wait for synchronization

  // Set the period (the number to count to (TOP) before resetting timer)
  TCC0->PER.reg = period;
  while (TCC0->SYNCBUSY.bit.PER);

  // Set PWM signal to output 50% duty cycle
  // n for CC[n] is determined by n = x % 4 where x is from WO[x]
  TCC0->CC[1].reg = period / 2;
  while (TCC0->SYNCBUSY.bit.CC2);

  // Configure PA15 (D5 on Arduino Zero) to be output
  PORT->Group[PORTA].DIRSET.reg = PORT_PA15;      // Set pin as output
  PORT->Group[PORTA].OUTCLR.reg = PORT_PA15;      // Set pin to low

  // Enable the port multiplexer for PA15
  PORT->Group[PORTA].PINCFG[15].reg |= PORT_PINCFG_PMUXEN;

  // Connect TCC0 timer to PA15. Function F is TCC0/WO[5] for PA15.
  // Odd pin num (2*n + 1): use PMUXO
  // Even pin num (2*n): use PMUXE
  PORT->Group[PORTA].PMUX[7].reg = PORT_PMUX_PMUXO_F;

  // Enable output (start PWM)
  TCC0->CTRLA.reg |= (TCC_CTRLA_ENABLE);
  while (TCC0->SYNCBUSY.bit.ENABLE);              // Wait for synchronization
}

void loop() {

  // Enable output (start PWM)
  TCC0->CTRLA.reg |= (TCC_CTRLA_ENABLE);
  while (TCC0->SYNCBUSY.bit.ENABLE);              // Wait for synchronization
  delay(100);
  // Disable output (start PWM)
  TCC0->CTRLA.reg &= ~(TCC_CTRLA_ENABLE);
  while (TCC0->SYNCBUSY.bit.ENABLE);              // Wait for synchronization

  // Set the period (the number to count to (TOP) before resetting timer)
  TCC0->PER.reg = period*4/3;
  while (TCC0->SYNCBUSY.bit.PER);

  // Set PWM signal to output 50% duty cycle
  // n for CC[n] is determined by n = x % 4 where x is from WO[x]
  TCC0->CC[1].reg = period*4/3 / 2;
  while (TCC0->SYNCBUSY.bit.CC2);
  delay(1000);
    // Enable output (start PWM)
  TCC0->CTRLA.reg |= (TCC_CTRLA_ENABLE);
  while (TCC0->SYNCBUSY.bit.ENABLE);              // Wait for synchronization
  delay(100);
  // Disable output (start PWM)
  TCC0->CTRLA.reg &= ~(TCC_CTRLA_ENABLE);
  while (TCC0->SYNCBUSY.bit.ENABLE);              // Wait for synchronization

  // Set the period (the number to count to (TOP) before resetting timer)
  TCC0->PER.reg = period/4*3;
  while (TCC0->SYNCBUSY.bit.PER);

  // Set PWM signal to output 50% duty cycle
  // n for CC[n] is determined by n = x % 4 where x is from WO[x]
  TCC0->CC[1].reg = period/4*3 / 2;
  while (TCC0->SYNCBUSY.bit.CC2);
  delay(1000);

}

No, for SAMD, millis(), micros(), delay() use the SysTick system timer.
https://forum.arduino.cc/index.php?topic=414989.0
https://www.keil.com/pack/doc/cmsis/Core/html/group__SysTick__gr.html

TC5 is used for Arduino tone() and assigns TC5’s interrupt for it.

All other timers are unused except many of the TCs and TCCs are used for PWM on the various pins capable of it, but interrupts aren’t used for this. The RGB LED on pins PA06, PA07 and PA09 would use TCC1/WO[0], TCC1/WO[1] and TCC1/WO[3] (or TCC0/WO[1]) respectively, so we would want to stay away from those.

I’d like to keep TCC0 for the PA15 audio output, as that’s what I’m currently using and TCC1 can be kept for LEDs.

1 Like

That’s fine. It should be relatively easy to switch to TC3/WO[1] (the only alternative), if it turns out to be a better choice.

2 Likes

I would presume it’s because a function hooked up as an interrupt subroutine is usually never actually called by the program.

If it were the responsibility of the user to set up interrupt subroutines by explicitly providing the address of a function then it would be easy to determine whether the function is used or not because the user would have to take the function’s address at some point (i.e. reference it).

But if it’s compiler magic that sets up the subroutine (e.g. by placing it into a particular section of the .elf file, and/or by autogenerating the code that sets up the interrupts) then the expected behaviour would be that the function would never be referenced from the user’s code, but the interrupt subroutine would still be set up correctly.

But you’re mostly right about the issue of library conflicts.
Theoretically it would be possible for two libraries to use the same interrupt, but not simultaneously, so setting up and removing the assigned interrupt subroutine would have to be coordinated somehow, and that could get quite awkward.


Somehow I had a feeling this sort of thing would be coming.

Something like this would have been better:

// Note: cannot be constexpr because of reinterpret_cast
Tc * const TC4 = reinterpret_cast<Tc *>(0x42003000UL);

Note that this means that only one of COUNT8, COUNT16 or COUNT32 can be ‘active’ at a given time.

If any one of those member variables is used before it’s been ‘activated’ by assigning to it then the program will invoke undefined behaviour.
https://en.cppreference.com/w/cpp/language/union#Member_lifetime

I suspected SYNCBUSY being uint8_t would be the case,
in which case using != 0 would be preferable to an implicit cast to bool.

I hasten to point out that using bit fields like this is a bad idea becuase the exact behaviour of bit fields is implementation defined (i.e. compiler specific):
https://en.cppreference.com/w/cpp/language/bit_field#Notes

If the code is only used with one compiler then it shouldn’t cause any problems, but it’s still bad form.

This should work, but to be safe I would suggest making the type of address a function pointer:

using TimerInterruptFunction = void (*)();
bool assignTimerInterrupt(TimerInterruptFunction function);
bool releaseTimerInterrupt();

Theoretically this should be portable.

That’s annoying to say the least.

Do you mean that assignTimerInterrupt(nullptr) would purposely make the interrupt subroutine do nothing?

Wouldn’t they be able to check whether the interrupt was already in use before assigning it?

For that matter, wouldn’t assignTimerInterrupt be able to do that as well?

Or is the interrupt table write-only?

I had a peek at the code for micros

I thought the stories I’d heard about !! were just horror stories about the dark ages, I didn’t realise people were actually still doing it:

!!(SCB->ICSR & SCB_ICSR_PENDSTSET_Msk)
Why !! ? The reason it works is because the first `!` (the inner `!`) converts the integer to a boolean, but it also inverts it, so a second `!` is needed to make it represent its intended value, hence two `!`s, forming `!!`.

As for why people insist on using it, your guess is as good as mine.
x != 0 is far more readable than !!x and achieves the same thing in fewer operations.

As for why not just use (bool)x,
to answer that you have to go back in time…

A little history lesson...

In the days of yore, before C was standardised, C didn’t have a bool type.

The inputs to if and while would be integers, which would be ‘false’ if the integer was 0 or ‘true’ for non-zero values.
(This is why you sometimes see people write while(1) instead of while(true).)

People realised that having a dedicated boolean type with true and false values was a good idea, so typically they’d create their own macros for BOOL, TRUE and FALSE.

Despite the fact that this practice was rampant,
it wasn’t until C99 that C finally got a standardised boolean type.
It was so late in the game that they ended up calling it _Bool to avoid clashing with anyone else’s definition.
They also added a new header called <stdbool.h> which provided bool, true and false as convenience macros.

(As for why macros were used instead of type aliases (typedef), presumably it was because macros can be detected with #if defined(BOOL)/#if defined(bool), and two macros with clashing names don’t automatically halt the compiler like two type aliases would.)

So basically unless you’re targetting pre-C99 compilers there’s no sensible reason to use !!, it’s just cryptic and archaic, and not in the poetic shakespearean way.

1 Like

I have got Arduboy2Beep working with SAMD21 timers and no interrupts. Have just pushed the commit to my fork of Arduboy2, if anyone wants to have a look (btw, I haven’t made the improvements @Pharap suggested yet - plenty of admin needed down the line to tidy up comments / TODO:s etc).

I have had a quick look around for a game which uses just Arduboy2 but not ArduboyTones as a stage test for the library but not managed to find one like that. If you know one off the top of your head (with source available) please let me know. In the meantime, I will finish the port of ArduboyTones and see if I can get a game.

For now, I have selected the TCC0 clock divisor as 256 because it seems that gives sufficient resolution and range to achieve the frequencies in ArduboyTonesPitches.h. Or at least the ones that a piezo speaker could achieve (not so good at the low end!). We’d need to keep this the same between Arduboy2Beep.cpp and ArduboyTones.cpp if I’m not mistaken.

Try “Humanity Revenge”:


In case you’re wondering how I found it…
(Also, yes, this is just an excuse to make another lame joke. :P)

Step 1: Use the forum’s search function to search ‘ArduboyBeep’
Step 2: Find the following comment by Keyboard Camper:

Step 3: Find the game’s GitHub in the main post
Step 4: ???
Step 5: Profit

The forum’s search function is severely underrated.

2 Likes

I’m pretty sure the header files are supposed to work with C as well as C++.

My examples were just to illustrate the concept. They weren’t meant to be complete or even syntactically correct. Even the names are subject to discussion.

It could, which I guess would make it equivalent to a “release”. The real ISR would use the address being set to nullptr to indicate that it wasn’t in use. In this case, it would probably be best to call Dummy_Handler, the default for unassigned interrupts.

This was in regards to libraries that weren’t written or modified to be “Arduboy aware”. Libraries that people have written for general Arduino use.


Regarding all your complaints and suggestions about poor coding practices:
Both the CMSIS code and Arduino are maintained under GitHub. You can submit PRs or raise issues if you wish. :wink:

Then:

Tc * const TC4 = (Tc *)(0x42003000UL);

Would provide the same result.

(From a C perspective the reliance on implementation defined behaviour is even worse. In C, only unsigned int, signed int, int and _Bool are allowed to be used as bit fields, the ability to use any other types is implementation-defined behaviour.)

Yes, and I was suggesting an implementation detail (which is also subject to discussion).

Which presumably would mean it would return false on AVR.

Yes, in which case my question still stands,
surely they’d be able to read the value stored in an interrupt vector and see that an interrupt subroutine was already assigned?

I’m not saying that they necessarily would,
I know how much programmers love to assume that they’ve got free reign over all the CPU resources,
but theoretically would they be able to check, or is there a mechanism that prevents them reading the values in the interrupt table?

Unfortunately I doubt they’d care enough about stylistic issues and readability to make any suggested changes.
They’re so inundated with PRs and issues that they probably only consider ones that make notable changes and/or require little debate or discussion.

The main reason I brought them up was to make it clear that they are setting a bad example, for the benefit of any other forum readers who might not know that the examples are bad, or why the examples are bad.


A theoretical implementation of assigTimerInterrupt:

using TimerInterruptFunction = void (*)();

void emptyHandler() {}

bool assignTimerInterrupt(TimerInterruptFunction function);
{
	if(function == nullptr)
	{
		// e.g. Default_Handler for ARM
		interruptTable[timerInterruptIndex] = emptyHandler;
		return true;
	}
	else if(interruptTable[timerInterruptIndex] == emptyHandler)
	{
		interruptTable[timerInterruptIndex] = function;
		return true;
	}
	
	return false;	
}

I don’t think we should even make the functions available for the AVR (until if/when we needed to use an interrupt under the AVR architecture). They would be conditionally compiled in.

The library would have to do this at run time. If it works like AVR, by then it would be too late. The compiler would give an error, due to two libraries trying to use the same interrupt, and abort.

This is assuming we even have a problem to begin with. It may be possible to overwrite the vector, with maybe the last one to be compiled/linked winning. I haven’t had time to investigate yet.

Does using the weak addribute:
(From cortex_handlers.c):

void TC4_Handler      (void) __attribute__ ((weak, alias("Dummy_Handler")));

Allow reassigning the vector if all other instances using it also use weak (as, for instance, Tone does)?
(From Tone.cpp):

void TC5_Handler (void) __attribute__ ((weak, alias("Tone_Handler")));