I’m currently taking an online embedded programming course. This was our 4th programming assignment. The device is Microchip(Atmel) XMEGA-A3BU Xplained Development Kit, Mfr. Part Number: ATXMEGAA3BU-XPLD, the short name is Atxmega256A3BU. The device has an 8 bit data bus and a 16 bit address bus. I'm doing the development on Windows 10 in Microchip Studio 7 (a very poor clone of Visual Studio). I'm building the code without optimization. The optimizer has a habit of optimizing out my timing loops.


The assignment is to alternate between 2 LEDs and if one of the buttons is pushed change the behavior so that the LED associated with that button is pushed is lit. Current LED 0 is associated with button SW1 and LED 1 is associated with button SW2. We are to use the TCC0 timer to alternate between the 2 LEDs. My question in addition to asking for a general review is the code for reading from and writing to a hardware address most often similar to the code in ReadReg() and WriteReg() in the code below? I'm asking about the pointer and not the volatile keyword. I've known the volatile keyword a long time.

Just FYI, the instructor really likes comments. If you are wondering about the change history, I am not using GIT for this class.

This assignment has been handed in.


 * reg_io_wrappers.h
 * Functions to read and write the hardware registers on the device.
 * Required for assignments from my class.
 * Created: 4/27/2022 4:20:26 PM
 *  Author: pacmaninbw
 * Change History:
 * 5/11/2022    Changed added include for stdint-gcc.h to reg_io_wrappers.h,
 *      changed input and output types to uint8_t. Easier to find and change
 *      than unsigned char, better representation.


#include <stdint-gcc.h>

extern  void WriteReg(uint16_t RegAddress, uint8_t Value);
extern  uint8_t ReadReg(uint16_t RegAddress);

#endif /* REG_IO_WRAPPERS_H_ */


 * reg_io_wrappers.c
 * Functions to read and write the hardware registers on the device.
 * Required for assignments from my class.
 * Created: 4/27/2022 4:08:13 PM
 *  Author: pacmaninbw
 * Change History:
 * 5/11/2022    Changed added include for stdint-gcc.h to reg_io_wrappers.h,
 *      changed input and output types to uint8_t. Easier to find and change
 *      than unsigned char, better representation.

#include "reg_io_wrappers.h"

/* Write data to a hardware register */
void WriteReg(uint16_t RegAddress, uint8_t Value)
    *((volatile unsigned char *)RegAddress) = Value;

 /* Read data from a hardware register */
uint8_t ReadReg(uint16_t RegAddress)
    // disable interrupts
    uint8_t return_val = *((volatile uint8_t *)RegAddress);
    // enable interrupts

    return return_val;


 * Programming Assignment 4: Add timing using Clock to LED On/Off using switches
 * 1. Create a code using Round Robin architecture to blink both LED’s when no
 *      switch is pressed like in previous assignment. 
 * 2. When any switch is pressed, then only blink the LED(s) associated to the
 *      switch number. Use the SW1 switch to control LED 0 and SW2 switch to
 *      control LED 1.
 * 3. Create a function that controls the blink operation.
 * 4. The main task loop should check the switch setting and make decisions 
 *      whether to blink both LED's on at a time OR only blink the LEDS associated
 *      with the switches that are pressed.
 * Change History
 * 05/11/2022 - Moved call to delay function from main round robin to service_leds().
 *      This will allow buttons to interrupt the delay loop and force an early
 *      from the service_leds() function.
 *      Added service_buttons() function.
 *      Added include for stdint-gcc.h so that uint8_t is defined, better than 
 *          putting unsigned char everywhere. Easier to modify if necessary.
 * 05/14/2022 - 05/15/2022 - Converted the delay subroutine to use the
 *      TCC0 timer/counter from a timing loop.
 * 05/16/2022 - Fixed bug in delay function, was not complementing the read back
 *      of the overflow flag. Moved the declaration of toggle into main(), there
 *      was no need for it to be global to the file.

 #include <stdint-gcc.h>

/* Include the hardware address macros. */
#include "devreg.h"
 * The I/O read and write wrapper functions are in a separate file, they will rarely
 * need to be recompiled.
#include "reg_io_wrappers.h"

 * Not including stdbool.h, using K & R solution instead.
#define TRUE 1
#define FALSE 0

#if 0
 * Used for debugging purposes.
static void lightRedLED(void)
    uint8_t pin4 = 0x01 << 4;
    WriteReg(PORTD_DIR_REG, pin4);
    WriteReg(PORTD_OUTTGL_REG, ~pin4);

 * Non-interrupt service function.
 * First determine if a button has been pushed, then determine which button
 * it is. Set the global button pushed flag.
 * The button read back is in bits 1 and 2 (pins 1 and 2). Since the toggle
 * is an index into an array this needs to change to bits 0 and 1 so right
 * shift 1 bit. Since the pins are low when the button is pushed we need to
 * get the complement to find the proper value.
static uint8_t service_buttons(uint8_t toggle)
    /*  Set PORTF direction to input
    uint8_t current_pin_read = ReadReg(BUTTON_VALUE_REG);   /* Read back if any buttons are pushed. */
    current_pin_read = current_pin_read >> 1;
    current_pin_read = ~current_pin_read & (uint8_t)0x03;

     * If no buttons are currently pushed return the current toggle value.
    current_pin_read = (current_pin_read)? current_pin_read : toggle;
    return current_pin_read;

     * Keep each display state to approximately 1 to 2 seconds. Hopefully 1000
     * loop executions equals 1 second. The delay function will be terminated
     * if the button status has changed. Use the TCC0 timer for the delay.
static void DelayUsingTCC0(uint8_t requestedDelay)
     * As per the instructions for this weeks lab,
     * 1) Set CTRLA for prescaling divide by 1024
     * 2) Since we want the counter to overflow based on only the 8 bits in
     *      the low counter register, set the high count register to all high.
     * 3) Set the desired delay value in the lower 8 bits of the counter.
     * 4) Make sure the overflow interrupt bit is cleared.
     * 5) Poll the clock interrupt flags to check for overflow conditions.
    WriteReg(TCC0_CTRLA, CLK_CTRL_OPTS_DIV1024);
    WriteReg(TCC0_COUNT_HI, 0xFF);
     * Subtract the desired delay from the overflow value for the lower
     * 8 bits of the timer/counter. The overflow should occur when the
     * lower 8 bits reaches 0xFF.
    WriteReg(TCC0_COUNT_LOW, 0xFF - requestedDelay);

     * When overflow occurs the overflow bit in the interrupt register goes low.
     * Poll the interrupt flags for overflow.
    volatile uint8_t noOverflow = TRUE;
        volatile uint8_t overFlowCheck = ReadReg(TCC0_INTFLAGS);
        noOverflow = ~overFlowCheck & OVFIF_MASK;

    /* Make sure there are not interrupts for the rest of the program. */

 * Non-interrupt service function.
 * Toggle the LEDs. First LED 0 then LED 1. 
#define PIN0 (uint8_t) 0x01
#define PIN1 (uint8_t) 0x02
#define PINS_0_AND_1 (uint8_t) 0x03
static void service_leds(uint8_t pin_index)
    uint8_t pins[] = {PIN0, PIN1, PINS_0_AND_1};
    uint8_t pin = pins[pin_index];

    WriteReg(LED_ENABLE_REG, pin);  /* Enable output on the specific pin(s) */
    WriteReg(LED_TOGGLE_REG, ~pin); /* Drive the pin low. */

    WriteReg(LED_ENABLE_REG, 0);    /* Disable output on the pin */

int main (void)
    uint8_t toggle = 0;

    while (TRUE)
     * Simple Round Robin Architecture without interrupts.
     * Buttons are a higher priority than the LEDs because the buttons provide
     * user input and require a better response time. The buttons also change
     * the status of which LEDs to display.
        toggle = service_buttons(toggle);
        toggle = (toggle)? 0 : 1;


 * devreg.h
 *  Portable Register Addressing to allow the code using this file to port to
 *  other devices.
 *  Required for assignments from my class.
 * Created: 4/27/2022 3:33:12 PM
 *  Author: pacmaninbw
 * Change History
 * Created 4/27/2022.Currently only contains macros to program the PORTR
 *      functionality, to enable and disable the LEDs. To make the LED
 *      programming more portable LED macros were added to hide the PORTR
 *      implementation on the device.
 * 5/1/2022. Changed the PORT Address macros implementation, Ports can be added
 *      by copy and paste, select region, find and replace base port register
 *      name.
 * 5/8/2022 Added PORTF Macros.
 * 5/10/2022 Converted LED constants to use the complement of decimal numbers
 *      rather than Hex values.
 * 5/14/2022 Added clock address offsets, CLOCK_TCC0 and the clock prescaler values
 *      for clock TCC0.
 * 5/16/2022 Added Port D addresses, corrected copy paste errors in Ports B & C.
 *      Attempting to use red LED for debugging.

#ifndef DEVREG_H_
#define DEVREG_H_

 * Clock Control Registers for TCC0
#define CLOCK_TCC0                          0x0800
#define CLOCK_CTRLA_OFFSET                  0x00
#define CLOCK_CTRLB_OFFSET                  0x01
#define CLOCK_CTRLC_OFFSET                  0x02
#define CLOCK_CTRLD_OFFSET                  0x03
#define CLOCK_CTRLE_OFFSET                  0x04
#define CLOCK_CTRLF_CLEAR_OFFSET            0X08
#define CLOCK_CTRLF_SET_OFFSET              0X09
#define CLOCK_CTRLG_CLEAR_OFFSET            0X0A
#define CLOCK_CTRLG_SET_OFFSET              0X0B
#define CLOCK_COUNTER_LOW_OFFSET            0X20
#define CLOCK_COUNTER_HI_OFFSET             0X21

 * Adding Clock Offsets
#define ADD_CLOCK_CTRLA_OFFSET(baseAddress) \
        (baseAddress + CLOCK_CTRLA_OFFSET)
#define ADD_CLOCK_CTRLB_OFFSET(baseAddress) \
        (baseAddress + CLOCK_CTRLB_OFFSET)
#define ADD_CLOCK_CTRLC_OFFSET(baseAddress) \
        (baseAddress + CLOCK_CTRLC_OFFSET)
#define ADD_CLOCK_CTRLD_OFFSET(baseAddress) \
        (baseAddress + CLOCK_CTRLD_OFFSET)
#define ADD_CLOCK_CTRLE_OFFSET(baseAddress) \
        (baseAddress + CLOCK_CTRLE_OFFSET)
        (baseAddress + CLOCK_INTERRUPT_CTRLA_OFFSET)
        (baseAddress + CLOCK_INTERRUPT_CTRLB_OFFSET)
#define ADD_CLOCK_CTRLF_CLEAR_OFFSET(baseAddress)   \
        (baseAddress + CLOCK_CTRLF_CLEAR_OFFSET)
#define ADD_CLOCK_CTRLF_SET_OFFSET(baseAddress) \
        (baseAddress + CLOCK_CTRLF_SET_OFFSET)
#define ADD_CLOCK_CTRLG_CLEAR_OFFSET(baseAddress)   \
        (baseAddress + CLOCK_CTRLG_CLEAR_OFFSET)
#define ADD_CLOCK_CTRLG_SET_OFFSET(baseAddress) \
        (baseAddress + CLOCK_CTRLG_SET_OFFSET)
        (baseAddress + CLOCK_INTERRUPT_FLAGS_OFFSET)
#define ADDCLOCK_COUNTER_LOW_OFFSET(baseAddress)    \
        (baseAddress + CLOCK_COUNTER_LOW_OFFSET)
#define ADD_CLOCK_COUNTER_HI_OFFSET(baseAddress)    \
        (baseAddress + CLOCK_COUNTER_HI_OFFSET)

 * Clock Register Addresses

 * TCC0 Clock Control Register Addresses

 * Clock Control Options and Prescaler Settings
#define CLK_CTRL_OPTS_OFF       (uint8_t) 0X00
#define CLK_CTRL_OPTS_DIV1      (uint8_t) 0X01
#define CLK_CTRL_OPTS_DIV2      (uint8_t) 0X02
#define CLK_CTRL_OPTS_DIV4      (uint8_t) 0X03
#define CLK_CTRL_OPTS_DIV8      (uint8_t) 0X04
#define CLK_CTRL_OPTS_DIV64     (uint8_t) 0X05
#define CLK_CTRL_OPTS_DIV256    (uint8_t) 0X06
#define CLK_CTRL_OPTS_DIV1024   (uint8_t) 0X07

 * Other Clock Control settings
#define CLEAR_OVFIF             (uint8_t) 0x01
#define OVFIF_MASK              (uint8_t) 0x01

// LED control values
// For this device there are 2 LEDs, so there are 4 LED states, all LEDs off,
// all LEDs on, LED 0 on, LED 1 on.
// The yellow LEDs are controlled by pins 0 and 1 in PORTR. The LED controlled
// by pin 0 will be called LED 0 and the LED controlled by pin 1 will be called
// LED 1. When driving low on either pin 0 or pin 1 the associated LED will
// light up.
#define ALL_LEDS_OFF        (uint8_t) ~0x00
#define ALL_LEDS_ON         (uint8_t) ~0x03
#define LED_0_ON            (uint8_t) ~0x01
#define LED_1_ON            (uint8_t) ~0x02
#define LED_MAX_STATES      4
#define LED_STATE_MASK      0x03 /* The 2 LSBs in the PORT R register */
#define ENABLE_ALL_LEDS     (uint8_t) 0x03
#define TURN_OFF_ALL_LEDS   (uint8_t) 0x03

#define PORT_DIR_OUTPUT     0xFF /* Page 148 of the Manual Bits 0 and 1 output */
#define PORT_DIR_INPUT      0xFC /* Bits 0 and 1 input */

// Device Port Address Offsets
// These offsets are documented on page 160 of the manual
// Atmel-8331-8-and-16-bit-AVR-Microcontroller-XMEGA-AU_Manual.pdf
#define PORT_DIR_OFFSET     0x00
#define PORT_DIRSET_OFFSET  0x01
#define PORT_OUT_OFFSET     0x04
#define PORT_OUTSET_OFFSET  0x05
#define PORT_OUTCLR_OFFSET  0x06
#define PORT_OUTTGL_OFFSET  0x07
#define PORT_IN_OFFSET      0x08

// Adding Device Port Offsets
#define ADD_PORT_DIR_OFFSET(baseAddress)    \
         (baseAddress + PORT_DIR_OFFSET)
#define ADD_PORT_DIRSET_OFFSET(baseAddress) \
        (baseAddress + PORT_DIRSET_OFFSET)
#define ADD_PORT_DIRCLR_OFFSET(baseAddress) \
        (baseAddress + PORT_DIRCLR_OFFSET)
#define ADD_PORT_DIRTGL_OFFSET(baseAddress) \
        (baseAddress + PORT_DIRTGL_OFFSET)
#define ADD_PORT_OUT_OFFSET(baseAddress)    \
        (baseAddress + PORT_OUT_OFFSET)
#define ADD_PORT_OUTSET_OFFSET(baseAddress) \
        (baseAddress + PORT_OUTSET_OFFSET)
#define ADD_PORT_OUTCLR_OFFSET(baseAddress) \
        (baseAddress + PORT_OUTCLR_OFFSET)
#define ADD_PORT_OUTTGL_OFFSET(baseAddress) \
        (baseAddress + PORT_OUTTGL_OFFSET)
#define ADD_PORT_IN_OFFSET(baseAddress)     \
        (baseAddress + PORT_IN_OFFSET)

/* PORT R Device Addresses */
#define PORTR_BASE_ADDRESS  0x07E0

/* PORT F Device Addresses */
#define PORTF_BASE_ADDRESS  0x06A0

/* PORT A Device Addresses */
#define PORTA_BASE_ADDRESS  0x0600

/* PORT B Device Addresses */
#define PORTB_BASE_ADDRESS  0x0620

/* PORT C Device Addresses */
#define PORTC_BASE_ADDRESS  0x0640

/* PORT D Device Addresses */
#define PORTD_BASE_ADDRESS  0x0660

 * Portable names for device registers does not require knowledge of device
#define LED_ENABLE_REG      PORTR_DIR_REG           

 * Buttons
 * The SW1 and SW2 button input is available on PORTF PIN1 and PIN2 respectively
 * as documented on page 10 of doc8394.pdf.
 #define SW1_AND_SW2_ARE_PRESSED    (uint8_t) ~0x06     // Pins 1 and 2 are low
 #define SW1_IS_PRESSED             (uint8_t) ~0x02     // Pin 1 is low
 #define SW2_IS_PRESSED             (uint8_t) ~0x04     // Pin 2 is low
 #define BUTTON_VALUE_REG           PORTF_IN_REG
 #define ENABLE_BUTTON_INPUT        (uint8_t) ~0x06

#endif /* DEVREG_H_ */

  [1]: https://i.sstatic.net/g0zPK.jpg
  • 2
    \$\begingroup\$ I am not using GIT for this class - OK; but.. you should? If the prof "doesn't allow it", what he doesn't know won't hurt him. \$\endgroup\$
    – Reinderien
    Commented May 16, 2022 at 21:56
  • 1
    \$\begingroup\$ @Reinderien It would make life a little easier. Each project builds on the last one. \$\endgroup\$
    – pacmaninbw
    Commented May 16, 2022 at 21:57
  • 1
    \$\begingroup\$ What is your clock frequency? \$\endgroup\$
    – Reinderien
    Commented May 16, 2022 at 22:31
  • 1
    \$\begingroup\$ @Reinderien I looked through the manual and could not find a definitive answer. I don't know the frequency and that prevents me from properly calculating the timer value. It is the internal clock on the device. \$\endgroup\$
    – pacmaninbw
    Commented May 16, 2022 at 22:35
  • 2
    \$\begingroup\$ @Reinderien Yes, it is the AVR flavour of gcc. I don't have any other AVR cross compilers installed. The current delay is no where near one second, somewhere between .25 and .5 seconds as far as I can tell. \$\endgroup\$
    – pacmaninbw
    Commented May 17, 2022 at 13:21

3 Answers 3


The optimizer has a habit of optimizing out my timing loops.

This is (a) a non-surprise, and (b) somewhat of a red flag when it comes to coding practice. If you're writing timing loops that attempt to take advantage of instruction durations to time events, expect difficulty unless you drop down to assembly (bypassing the compiler and optimiser), or do the saner thing and use hardware timers when possible.

Regarding ReadReg and WriteReg. Don't treat addresses as uint16_t. You'll need to read your compiler manual to rule out weird behaviour like near/far modifiers etc., but if humanly possible, those RegAddress should be actual * pointers. You cast to a volatile unsigned char *: check if that's the size you're looking for, and if so, just use that as your argument type.

More on the above: as is quite typical with microcontrollers, they have an address space that's more complicated in some ways to use than modern desktop architectures. Read chapter 3 AVR CPU, particularly

Direct addressing of up to 16MB of program memory and 16MB of data memory

The memory spaces are linear. The data memory space and the program memory space are two different memory spaces.

All I/O status and control registers reside in the lowest 4KB addresses of the data memory. This is referred to as the I/O memory space. The lowest 64 addresses can be accessed directly, or as the data space locations from 0x00 to 0x3F. The rest is the extended I/O memory space, ranging from 0x0040 to 0x0FFF. I/O registers here must be accessed as data space locations using load (LD/LDS/LDD) and store (ST/STS/STD) instructions.

and Chapter 4 Memories

  • Data memory
    • One linear address space
    • Single-cycle access from CPU
    • SRAM
    • EEPROM
      • Byte and page accessible
      • Optional memory mapping for direct load and store
    • I/O memory
      • Configuration and status registers for all peripherals and modules
      • 16 bit-accessible general purpose registers for global variables or flags

[...] All memory spaces are linear and require no memory bank switching.

Not all addresses are created equal. Addresses in the lower 64B use a different access mechanism from those above. Read this section in GCC for evidence that gcc treats AVR specially. Generically speaking, gcc may (though doesn't appear to) have chosen to add a near prefix for pointers to this lower segment, and far for pointers to this upper segment - this is done in other compilers for other architectures. Since I don't think it does, I encourage you to

  • Rewrite your macros e.g. #define PORTD_DIR_REG ((volatile unsigned char*)0x0660)
  • Get rid of your ReadReg/WriteReg
  • Just *PORTD_DIR_REG = 0x...;
  • Manually verify the compiled assembly to ensure that it makes sense based on the destination address

I don't understand the comment // disable interrupts. Because.. you don't do that? Or is this some magic the compiler does for you to wrap volatile dereferences? You should specify.

Set PORTF direction to input seems like something that should be done once on initialisation and then left alone.

    current_pin_read = (current_pin_read)? current_pin_read : toggle;
    return current_pin_read;

is a pretty surprising way of writing

    return current_pin_read || toggle;

The delay function will be terminated if the button status has changed.

Polling loops are a method of last resort. Hopefully your dev board has wired the button to a pin supporting hardware interrupt. Based on the documentation, your button pins are PE5, PF1 and PF2. In your microcontroller manual chapter 13.6, it shows

Two port interrupts with pin masking per I/O port

Enable that hardware interrupt.

As a matter of some urgency, you need to know what your clock rate is (ask your prof?). Then, there will be no manual iterations, and a single setting of the timer with a single expiry.

You claim that the current delay is somewhere between 250-500 ms for your timer value of 250. This suggests a system clock between

$$ 1024 * 250 / 0.250 = 1.024 \text{MHz} $$


$$ 1024 * 250 / 0.500 = 512 \text{kHz} $$

Visual inspection only reveals an RTC-style 32 kHz crystal and no other hardware oscillator. Between this and the fact that the 2 MHz internal oscillator is the default clock source used on startup, chances are that your base oscillator is 2 MHz, depicted in the bottom right:


It's less likely that XOSCSEL and PLLSRC are set such that your system clock is some multiple of the 32,768 Hz crystal. A PLL factor of 15-31 would be consistent with the delay you're seeing, but again, if you haven't chosen this explicitly, you're just on 2 MHz.

TC0 in figure 14-1 only shows an input of clkPER, so it's unlikely that the system prescaler has any effect, which in turn increases the likelihood that you're on 2 MHz or below. You should attempt to nail this down before your next assignment.

For an expiry of one second, you cannot use your timer's lower half only. You need to use all 16 bits. If you add a macro that - rather than pointing to a uint8_t, points to a uint16_t - and you get your endianness right, a simple dereference-and-assign should compile to the right thing.

Assuming for now that your system clock of 2 MHz is accurate, with a 16-bit-wide counter and your available prescaler settings, your prescaler feasibility region looks like:


The timer should be "set-and-forget". Refer to chapter 14.8.1 Waveform Generation - the output should be automatic, and you should only enact a port override if you get a button interrupt.

Then put the CPU into an idle or sleep state with no loops required. By chapter 8.2:

When the device enters sleep mode, program execution is stopped and interrupts or a reset is used to wake the device again.

  • \$\begingroup\$ I should have removed the interrupt comment, since we aren't using interrupts yet. I hope to learn how, but we haven't been taught how to use the interrupt vector table yet, nor have we been taught how to store the interrupt sections separately. \$\endgroup\$
    – pacmaninbw
    Commented May 16, 2022 at 22:29
  • 1
    \$\begingroup\$ Cool... best practice is best practice. You'll learn how to use interrupts eventually. \$\endgroup\$
    – Reinderien
    Commented May 16, 2022 at 22:30
  • \$\begingroup\$ Where can I read more about near/far modifiers? \$\endgroup\$
    – pacmaninbw
    Commented May 17, 2022 at 12:31
  • 1
    \$\begingroup\$ I examine the generated assembly religiously every time I compile. I don't trust compilers that optimize out loops. I've caught the fact that the code is optimized twice by examining the assembly. \$\endgroup\$
    – pacmaninbw
    Commented May 17, 2022 at 18:18
  • 5
    \$\begingroup\$ "I examine the generated assembly religiously every time I compile." I often do this, too. "I don't trust compilers that optimize out loops." I don't trust compilers that don't optimize out do-nothing loops! If you don't want optimized code, don't enable optimizations. If you do, then you should get the best optimizations the compiler can produce. Loops are not the way to insert delays. Ever. Full stop. That code is wrong. Not "not a best practice", but completely wrong. \$\endgroup\$ Commented May 17, 2022 at 21:55

Portable Register Addressing to allow the code using this file to port to other devices.

I seriously doubt that. Other devices may have (and will have) completely different mapping. What's worse, the mapping of external hardware (such as LEDs and buttons) to particular pins is specific to the board, but not to the device.

This file may increase readability, but it doesn't address portability at all. You may consider to add another abstraction layer as, say, readButtonStatus(...), setLED(...), etc.

Edit to clarify: The pin is a property of the controller. It is a bit X in a port Y. What hardware a pin is wired to is a property of the board. The controller support and the board support are different beasts.

Buttons are a higher priority

The code does not support this claim. service_leds calls DelayUsingTCC0, which runs the delay loop to completion, so until the delay completes buttons will not be served. In an infinite loop the order of calls doesn't matter.

It is very unclear how the application should behave if both buttons are pressed. If I understand this code correctly, the implementation prefers the state of PIN1. Perhaps it is an expected behaviour, perhaps not. In any case it shall be documented.

  • \$\begingroup\$ Could you say more about the the implementation prefers the state of PIN1. It does seem to prefer LED1 over LED0. \$\endgroup\$
    – pacmaninbw
    Commented May 17, 2022 at 12:29

Use a standard library

It looks like you have written a lot of code to deal with AVR registers. Maybe it was required that you wrote everything from scratch, but it would be much better if you could use a standard library for interfacing with AVR microcontrollers. There's Arduino of course, but I recommend using AVR Libc, which seems to be included with Atmel Studio 7. It exposes all the registers of your device, correctly made volatile, such that you can write:

#include <avr/io.h>

static void lightRedLED(void)
    PORTD_DIR = PIN4_bm;
    PORTD_OUTTGL = ~PIN4_bm;

Why not use stdbool.h?

GCC for AVR comes with stdbool.h, I don't see why you are defining TRUE and FALSE manually? There is no law against using modern programming standards when it comes to embedded devices, you don't have to resort to K&R C.

  • \$\begingroup\$ The library includes also define all the addresses, one of the instructors goals seems to be to teach us some best habits by having separate header file with address macros. He also specifically told us we couldn't use the delay function from the library. He has had us remove the library includes from each project. Re: stdbool.h, I don't know if it adds any code and I'm trying to keep the code as small as possible. \$\endgroup\$
    – pacmaninbw
    Commented May 17, 2022 at 11:37
  • 4
    \$\begingroup\$ stdbool.h does not add any code. You can check for yourself what is in that header file. I'm not sure what the instructors are trying to teach you. If it's writing a program that runs on a microcontroller, then I would've told you to use <avr/io.h> and drop everything except the code in your main.c. If they want you to learn how to write a support library from scratch, then sure. I just want to mention this in case someone else reads this post and wonders whether they need to write all the register definitions themselves if their goal is just to toggle some LEDs :) \$\endgroup\$
    – G. Sliepen
    Commented May 17, 2022 at 12:14
  • \$\begingroup\$ As far as I can tell one of the things we are supposed to learn is how to read datasheets and manuals. I just looked at this weeks assignment, we start using the library, we need it to use the timer interrupts. I agree with your last sentence, there are library routines for almost everything, it is better to use debugged software. \$\endgroup\$
    – pacmaninbw
    Commented May 17, 2022 at 12:27
  • 3
    \$\begingroup\$ "There is no law against using modern programming standards when it comes to embedded devices." As someone who does embedded development professionally, I absolutely cannot emphasize this enough. I don't know how this fiction got started, that embedded systems can't use modern programming tools and practices, but it's definitely just that: a fiction. Update your compilers; update your toolchains. Use everything they have to offer. It probably matters more on embedded than it does anywhere else. And there is no reason to ever compromise in the name of compatibility, as you control it all! \$\endgroup\$ Commented May 17, 2022 at 21:57
  • 3
    \$\begingroup\$ You might, for example, normally seek to write portable code that can be compiled on a variety of platforms, and thus you might not adopt the bleeding-edge language standards. Not a concern in the embedded world, as that machine-specific code isn't ever going to run on anything else, so why not use the latest tools that make your job easier, make your code better, and eke out the maximum performance? Definitely don't stop asking the question, "will this bloat my code?", but learn to investigate and find out for yourself. Look at the code; look at the disassembly. \$\endgroup\$ Commented May 17, 2022 at 22:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.