4
\$\begingroup\$

The provided code controls a 3-phase heater by generating a PWM signal (in software) which controls 3 triacs that are connected to the heater. Between the MCU (running Zephyr RTOS) and the triacs is a PCF8574 IO extender. The idea is to simply tell the heater module which percentage of power is desired.

Furthermore, phase-shifting is implemented to reduce big transitions in current-draw. Also the triacs should be as evenly loaded as possible such that they degrade similarly over time. This is handled in PWM_Start().

In the code I have added // CODE REVIEW COMMENT: to give some context where perhaps relevant, like for Zephyr-specific things.

While writing this code I already had some doubts:

  1. Should I use on/off and start/stop functions, or have parameters for this?
  2. Am I using the ASSERTS correctly or should I just check it in-code?
  3. Should I prevent the timers from running in the boundary cases of 0% and 100% duty cycle? I feel like maybe it makes the code less generic for only a small benefit?
  4. I should probably split the Phase-shifted PWM code to a dedicated file, but I'm afraid this will require callback functions to be called at the PWM edges. I'm struggling with the trade-off, as the PWM code is only used here at the moment.

Notes

  1. __ASSERT is provided by Zephyr.

  2. k_work_init() is:

    static inline void k_work_init(struct k_work *work,  k_work_handler_t handler)
    {
      *work = (struct k_work)Z_WORK_INITIALIZER(handler);
    }
    
  3. k_work_handler_t is:

    typedef void (*k_work_handler_t)(struct k_work *work);
    

The code

Implementation .c file

/*********************************************************************
 * INCLUDES
 */

#include <zephyr.h>

#include "heater.h"

#include "pcf8574.h"

#include <logging/log.h>
LOG_MODULE_REGISTER(Heater, LOG_LEVEL_DBG);

/*********************************************************************
 * DEFINES
 */

// Time (in seconds) between phase activations
#define TIME_BETWEEN_PHASES   3

#define PWM_PERIOD            (TRIAC_COUNT * TIME_BETWEEN_PHASES)

/*********************************************************************
 * MACROS
 */

/*********************************************************************
 * TYPEDEFS
 */

typedef struct
{
  struct k_timer timer;
  bool           active;
}
S_TriacInfo;

typedef enum
{
  TRIAC_R = 0,
  TRIAC_S,
  TRIAC_T,

  TRIAC_COUNT,
}
E_Triacs;

typedef struct
{
  struct k_work work;
  E_Triacs      triac;
}
S_PWMWork;

/*********************************************************************
 * CONSTANTS
 */

static const E_PCF8574Ports triacToPortMapping[TRIAC_COUNT] = { PCF8574_P4, PCF8574_P5, PCF8574_P6 };

/*********************************************************************
 * PUBLIC VARIABLES
 */
 
/*********************************************************************
 * LOCAL VARIABLES
 */

static S_TriacInfo triacs[TRIAC_COUNT];
static uint8_t     dutyCycle;

static S_PWMWork   flipPWM;

/*********************************************************************
 * LOCAL FUNCTION PROTOTYPES
 */

static void TriacTimerFxn (struct k_timer *timer);

static void FlipPWM (struct k_work *item);

static bool PWM_Start (void);
static bool PWM_StartHighCycle (E_Triacs triac);
static bool PWM_StartLowCycle  (E_Triacs triac);

static bool TurnOnTriac  (E_Triacs triac);
static bool TurnOffTriac (E_Triacs triac);

/*********************************************************************
 * LOCAL FUNCTIONS
 */

static void TriacTimerFxn (struct k_timer *timer)
{
  for (E_Triacs triac = TRIAC_R; triac < ARRAY_SIZE(triacs); triac++)
  {
    if (timer == &triacs[triac].timer)
    {
      flipPWM.triac = triac;
      k_work_submit(&flipPWM.work);
      break;
    }
  }
}

static void FlipPWM (struct k_work *item) // CODE REVIEW COMMENT: This function follows a library typedef, it's always a void
{
  S_PWMWork *pPWMWork = CONTAINER_OF(item, S_PWMWork, work);
  E_Triacs triac = pPWMWork->triac;

  if (triacs[triac].active)
    PWM_StartLowCycle(triac);
  else
    PWM_StartHighCycle(triac);
}

static bool PWM_Start (void)
{
  static E_Triacs currentFirstTriac = TRIAC_R;

  // Start PWM cycle for the first triac
  if (!PWM_StartHighCycle(currentFirstTriac))
  {
    LOG_ERR("Failed to start PWM");
    return false;
  }

  // Start timers to support phase-shifting for the other triacs
  for (uint8_t i = 1; i < ARRAY_SIZE(triacs); i++)
  {
    E_Triacs triac = (currentFirstTriac + i) % ARRAY_SIZE(triacs);

    if (!TurnOffTriac(triac))
    {
      LOG_ERR("Failed to start PWM");
      return false;
    }

    uint16_t timer_ms = 1000 * (triac * TIME_BETWEEN_PHASES);
    k_timer_start(&triacs[triac].timer, K_MSEC(timer_ms), K_SECONDS(0));
  }

  currentFirstTriac = (currentFirstTriac + 1) % ARRAY_SIZE(triacs);

  LOG_DBG("PWM started");

  return true;
}

static bool PWM_StartHighCycle (E_Triacs triac)
{
  __ASSERT(triac >= TRIAC_R &&
           triac <  ARRAY_SIZE(triacs),
           "Invalid triac");

  if (!TurnOnTriac(triac))
  {
    LOG_ERR("Failed to start high cycle for triac %u", triac);
    return false;
  }

  uint16_t timer_ms = 1000 * PWM_PERIOD * dutyCycle / 100;
  k_timer_start(&triacs[triac].timer, K_MSEC(timer_ms), K_SECONDS(0));

  LOG_DBG("Started high cycle for triac %u (%u ms)", triac, timer_ms);

  return true;
}

static bool PWM_StartLowCycle (E_Triacs triac)
{
  __ASSERT(triac >= TRIAC_R &&
           triac <  ARRAY_SIZE(triacs),
           "Invalid triac");

  if (!TurnOffTriac(triac))
  {
    LOG_ERR("Failed to start low cycle for triac %u", triac);
    return false;
  }

  uint16_t timer_ms = 1000 * PWM_PERIOD * (100 - dutyCycle) / 100;
  k_timer_start(&triacs[triac].timer, K_MSEC(timer_ms), K_SECONDS(0));

  LOG_DBG("Started low cycle for triac %u (%u ms)", triac, timer_ms);

  return true;
}

static bool TurnOnTriac (E_Triacs triac)
{
  __ASSERT(triac >= TRIAC_R &&
           triac <  ARRAY_SIZE(triacs),
           "Invalid triac");

  E_PCF8574Ports port = triacToPortMapping[triac];
  if (!PCF8574_ConfigurePin(port, PCF8574_OUTPUT_LOW))
  {
    LOG_ERR("Failed to turn on triac %u", triac);
    return false;
  }

  LOG_DBG("Turned on triac: %u", triac);
  triacs[triac].active = true;

  return true;
}

static bool TurnOffTriac (E_Triacs triac)
{
  __ASSERT(triac >= TRIAC_R &&
           triac <  ARRAY_SIZE(triacs),
           "Invalid triac");

  E_PCF8574Ports port = triacToPortMapping[triac];
  if (!PCF8574_ConfigurePin(port, PCF8574_OUTPUT_HIGH))
  {
    LOG_ERR("Failed to turn off triac %u", triac);
    return false;
  }

  LOG_DBG("Turned off triac: %u", triac);
  triacs[triac].active = false;

  return true;
}

/*********************************************************************
 * PUBLIC FUNCTIONS
 */

bool Heater_Init (void) // CODE REVIEW COMMENT: This is a bool so it follows the style of all other modules, currently there are no fault conditions
{
  LOG_DBG("Initializing heater");

  for (E_Triacs triac = TRIAC_R; triac < ARRAY_SIZE(triacs); triac++)
  {
    k_timer_init(&triacs[triac].timer, TriacTimerFxn, NULL);
    triacs[triac].active = false;
  }

  k_work_init(&flipPWM.work, FlipPWM);

  return true;
}

bool Heater_SetPower (uint8_t powerPercentage)
{
  __ASSERT(powerPercentage <= 100, "Invalid power percentage");

  LOG_DBG("Setting heater power: %u%%", powerPercentage);

  dutyCycle = powerPercentage;

  return PWM_Start();
}

Header .h file

#ifndef HEATER_H
#define HEATER_H

/*********************************************************************
 * INCLUDES
 */

#include <stdint.h>

/*********************************************************************
 * DEFINES
 */

/*********************************************************************
 * MACROS
 */

/*********************************************************************
 * TYPEDEFS
 */

/*********************************************************************
 * CONSTANTS
 */

/*********************************************************************
 * PUBLIC VARIABLES
 */

/*********************************************************************
 * PUBLIC FUNCTIONS
 */

bool Heater_Init (void);

bool Heater_SetPower (uint8_t powerPercentage);

#endif // HEATER_H
\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

Consider using Doxygen

I see you have some structured way of documenting your code using comments. However, I recommend you use Doxygen's format to document your code. This ensures you still have human-readable comments documenting your code, but it also comes with tools that can generate manuals from the comments in your source code, as well as verify that you didn't forget to document all functions, parameters and variables.

Avoid macros for constants

Instead of using #define for constants, declare static const variables. The advantage is that they will have a proper type, and you avoid potential issues if you have epxressions but forgot to add parentheses (you did it correctly in your code though). So for example:

static const uint16_t TIME_BETWEEN_PHASES = 3;
static const uint16_t PWM_PERIOD = TRIAC_COUNT * TIME_BETWEEN_PHASES;

Don't overuse enums

While enums are usually good to use, there are some cases where they can be problematic. For example in this line:

for (E_Triacs triac = TRIAC_R; triac < ARRAY_SIZE(triacs); triac++)

Are you sure that TRIAC_R is the first triac? What if someone reorders the declaration of E_Triacs? Since you just want to iterate over all the elements of triacs, I would not use an enum for the loop iterator here, but rather a regular integer, and start at 0 explicitly:

for (uint16_t triac = 0; triac < ARRAY_SIZE(triacs); triac++)

Looking at the rest of the code, TRIAC_S and TRIAC_T are never used, and even TRIAC_COUNT is not used a lot. This tells me you are not really interested in giving each triac a symbolic name, they are just three triacs. So I would just use a regular integer:

enum {
    TRIAC_COUNT = 3, // So we can still use it to declare arrays
};

typedef struct
{
  struct k_work work;
  uint16_t      triac;
}
S_PWMWork;

It also simplifies some of the asserts if you keep the triac number in an unsigned int, for example:

__ASSERT(triac >= TRIAC_R && triac <  ARRAY_SIZE(triacs), "Invalid triac");

Becomes:

__ASSERT(triac <  ARRAY_SIZE(triacs), "Invalid triac");

That assert was a bit weird anyway, since you already assumed that the triacs were numbered starting with 0 in statements like these:

E_Triacs triac = (currentFirstTriac + i) % ARRAY_SIZE(triacs);

About error checking

There is a lot of error checking happening in your code. Let's look at:

if (!PCF8574_ConfigurePin(port, PCF8574_OUTPUT_LOW))

I am surprised by this; would setting a port low or high ever be able to fail on your MCU? And the return value of PWM_StartLowCycle() and PWM_StartHighCycle() are ignored in FlipPWM() anyway. What is the point of error checking if you don't do anything with the error?

Either it can never fail while running (although perhaps you could do a simple one-shot test at the start of your program to ensure the pins and ports are configured correctly), in which case I would not perform these error checks, or it can fail, in which case I would add some code to address the situation. Think about what can go wrong if a triac gets stuck in the on or off position. Could it overheat? If it stops heating, could something freeze? Is there a way to get an alarm signal out to notify a human to intervene? Is there a safe mode you could go into while the problem persists?

Consider merging start/stop functions into one

Indeed, you were thinking about whether you should have separate start and stop functions versus one function that takes a parameter. Consider that a lot of the code in PWM_StartHighCycle() and PWM_StartLowCycle() is the same, so there is a lot of duplication that can be removed. The same goes for TurnOnTriac() and TurnOffTriac(). So I would indeed try to merge these functions so they look like:

static void PWM_StartCycle(uint16_t triac, bool high) {
    ...
    SetTriac(triac, high);
    ...
}

static void SetTriac(uint16_t triac, bool on) {
    ...
}

This would also simplify FlipPWN():

static void FlipPWM (struct k_work *item) {
  S_PWMWork *pPWMWork = CONTAINER_OF(item, S_PWMWork, work);
  E_Triacs triac = pPWMWork->triac;
  PWM_StartCycle(triac, !trias[triac].active);
}

Handling boundary cases

If 0% and 100% duty cycle are valid operating points, then I would ensure these are handled correctly. If you don't special case them, what would happen is that you still flip between on/off, but for example at 0% you will then still have a very small time where it is on. Could this be an issue? Would that wear out the triac or heater? It would be relatively simple to check for this situation and avoid the additional stress on the system.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ When changing macros to constants, it's a good idea to change the names, so that only macros are ALL_CAPS. That helps us maintain awareness of which identifiers don't obey the usual rules of scope. \$\endgroup\$ Mar 7, 2021 at 11:41
  • \$\begingroup\$ Strongly disagree that Doxygen can generate manuals. It can summarize all your in-source comments, but if those are verbose enough to make up the contents of a manual, you are doing something very wrong. Similarly, brief stuff like /* this does x and then y */ are helpful inside the source but not elsewhere. Doxygen in general is abused a lot when people are too lazy to write proper documentation. I've yet to see any sufficient documentation coming out of any Doxygen project. Also, Doxygen is most definitely not a replacement for code templates. \$\endgroup\$
    – Lundin
    Mar 10, 2021 at 7:57
  • \$\begingroup\$ "Avoid macros for constants" This is an embedded system. Turning macros into constants may create all kinds of problems on Harvard architectures. Also, when you have more complex equations inside constants, those need to be in the form of integer constant expressions. You cannot do const int x=1; const int y=x+1; in C, since in order for constants to make sense on an embedded system, they must always have static storage duration and get allocated in .rodata, never on the stack. This static const uint16_t PWM_PERIOD = TRIAC_COUNT * TIME_BETWEEN_PHASES; won't compile and is misguided. \$\endgroup\$
    – Lundin
    Mar 10, 2021 at 8:02
  • \$\begingroup\$ @Lundin About Doxygen, I'm merely pointing out that using a widely used standard to format your comments in has potential advantages. I agree you can't just document functions and call that a manual, but it does automatically generate the reference part of the manual for you. Also, you can use Doxygen formatting in your code templates, for example using \section commands in each header comment, although Doxygen will already be able to split typedefs, functions, macros etc. by itself without needing that. \$\endgroup\$
    – G. Sliepen
    Mar 10, 2021 at 16:15
  • \$\begingroup\$ @Lundin As for constants: it doesn't matter if it's an embedded system or not. If you declare a static const (note the static), then compilers do not have to allocate anything in .rodata, and in fact they don't, see for example: godbolt.org/z/jKEGej. You're right about the expression not compiling though, I was probably thinking about C++'s constexpr constants. \$\endgroup\$
    – G. Sliepen
    Mar 10, 2021 at 16:17

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.