Предоставленный код управляет 3-фазным нагревателем, генерируя сигнал ШИМ (в программном обеспечении), который управляет 3 симисторами, подключенными к нагревателю. Между MCU (работающим под управлением Zephyr RTOS) и симисторами находится расширитель ввода-вывода PCF8574. Идея состоит в том, чтобы просто сообщить модулю нагревателя, какой процент мощности требуется.
Кроме того, реализован фазовый сдвиг, чтобы уменьшить большие переходы в потребляемом токе. Кроме того, симисторы должны быть по возможности равномерно нагружены, чтобы они одинаково деградировали со временем. Это обрабатывается в PWM_Start()
.
В коде я добавил // CODE REVIEW COMMENT:
чтобы дать некоторый контекст, где это возможно, например, для конкретных вещей Zephyr.
При написании этого кода у меня уже были некоторые сомнения:
- Должен ли я использовать функции включения / выключения и запуска / остановки или иметь параметры для этого?
- Я использую
ASSERTS
правильно или я должен просто проверить это в коде? - Должен ли я предотвратить запуск таймеров в граничных случаях 0% и 100% рабочего цикла? Я чувствую, что, может быть, это делает код менее универсальным с небольшой пользой?
- Я, вероятно, должен разделить код PWM со сдвигом фазы в специальный файл, но я боюсь, что это потребует вызова функций обратного вызова на краях PWM. Я борюсь с компромиссом, так как код PWM используется только здесь на данный момент.
Примечания
__ASSERT
является предоставлено Zephyr.k_work_init()
является:static inline void k_work_init(struct k_work *work, k_work_handler_t handler) { *work = (struct k_work)Z_WORK_INITIALIZER(handler); }
k_work_handler_t
является:typedef void (*k_work_handler_t)(struct k_work *work);
Код
Реализация .c файл
/*********************************************************************
* 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();
}
Заголовочный файл .h
#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
1 ответ
Рассмотрите возможность использования Doxygen
Я вижу, у вас есть какой-то структурированный способ документирования кода с помощью комментариев. Однако я рекомендую вам использовать Doxygenформат для документирования вашего кода. Это гарантирует, что у вас по-прежнему есть удобочитаемые комментарии, документирующие ваш код, но он также поставляется с инструментами, которые могут создавать руководства из комментариев в вашем исходном коде, а также проверять, не забыли ли вы задокументировать все функции, параметры и переменные.
Избегайте макросов для констант
Вместо того, чтобы использовать #define
для констант объявите static const
переменные. Преимущество в том, что они будут иметь правильный тип, и вы избежите потенциальных проблем, если у вас есть epxressions, но вы забыли добавить круглые скобки (хотя вы сделали это правильно в своем коде). Так например:
static const uint16_t TIME_BETWEEN_PHASES = 3;
static const uint16_t PWM_PERIOD = TRIAC_COUNT * TIME_BETWEEN_PHASES;
Не злоупотребляйте enum
s
Пока enum
s обычно удобны в использовании, но в некоторых случаях они могут быть проблематичными. Например, в этой строке:
for (E_Triacs triac = TRIAC_R; triac < ARRAY_SIZE(triacs); triac++)
Ты уверен что TRIAC_R
это первый симистор? Что, если кто-то изменит порядок декларации E_Triacs
? Поскольку вы просто хотите перебрать все элементы triacs
, Я бы не стал использовать enum
для итератора цикла здесь, а скорее обычное целое число, и начинаются с 0
явно:
for (uint16_t triac = 0; triac < ARRAY_SIZE(triacs); triac++)
Глядя на остальную часть кода, TRIAC_S
и TRIAC_T
никогда не используются, и даже TRIAC_COUNT
не используется много. Это говорит о том, что вам не очень интересно давать каждому симистору символическое имя, это всего лишь три симистора. Поэтому я бы просто использовал обычное целое число:
enum {
TRIAC_COUNT = 3, // So we can still use it to declare arrays
};
typedef struct
{
struct k_work work;
uint16_t triac;
}
S_PWMWork;
Это также упрощает некоторые утверждения, если вы сохраняете номер симистора в беззнаковом int, например:
__ASSERT(triac >= TRIAC_R && triac < ARRAY_SIZE(triacs), "Invalid triac");
Становится:
__ASSERT(triac < ARRAY_SIZE(triacs), "Invalid triac");
В любом случае это утверждение было немного странным, поскольку вы уже предположили, что триаки были пронумерованы, начиная с 0
в таких заявлениях:
E_Triacs triac = (currentFirstTriac + i) % ARRAY_SIZE(triacs);
О проверке ошибок
В вашем коде происходит много проверок ошибок. Давайте посмотрим на:
if (!PCF8574_ConfigurePin(port, PCF8574_OUTPUT_LOW))
Меня это удивляет; может ли установка порта на низкий или высокий уровень когда-либо выйти из строя на вашем MCU? И возвращаемое значение PWM_StartLowCycle()
и PWM_StartHighCycle()
игнорируются в FlipPWM()
так или иначе. Какой смысл проверять ошибки, если вы ничего не делаете с ошибкой?
Либо он никогда не может выйти из строя во время работы (хотя, возможно, вы могли бы провести простой однократный тест в начале своей программы, чтобы убедиться, что контакты и порты настроены правильно), и в этом случае я бы не стал выполнять эти проверки ошибок, либо он может сбой, и в этом случае я бы добавил код для разрешения ситуации. Подумайте, что может пойти не так, если симистор застрянет во включенном или выключенном положении. Мог ли он перегреться? Если он перестанет нагреваться, может что-нибудь замерзнуть? Есть ли способ получить сигнал тревоги, чтобы уведомить человека о вмешательстве? Есть ли безопасный режим, в который можно было бы перейти, пока проблема не исчезнет?
Рассмотрите возможность объединения функций запуска / остановки в одну
Действительно, вы думали о том, следует ли иметь отдельные функции запуска и остановки или одну функцию, которая принимает параметр. Учтите, что большая часть кода в PWM_StartHighCycle()
и PWM_StartLowCycle()
то же самое, поэтому есть много дублирования, которое можно удалить. То же самое касается TurnOnTriac()
и TurnOffTriac()
. Поэтому я бы действительно попытался объединить эти функции, чтобы они выглядели так:
static void PWM_StartCycle(uint16_t triac, bool high) {
...
SetTriac(triac, high);
...
}
static void SetTriac(uint16_t triac, bool on) {
...
}
Это также упростило бы 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);
}
Обработка пограничных случаев
Если рабочий цикл 0% и 100% являются допустимыми рабочими точками, я бы позаботился о том, чтобы они обрабатывались правильно. Если вы не сделаете их особенным, произойдет то, что вы все равно будете переключаться между включением / выключением, но, например, при 0% у вас все еще будет очень небольшое время, когда он включен. Может ли это быть проблемой? Износит ли это симистор или нагреватель? Было бы относительно просто проверить эту ситуацию и избежать дополнительной нагрузки на систему.
При изменении макросов на константы рекомендуется изменить имена, чтобы можно было использовать только макросы.
ALL_CAPS
. Это помогает нам поддерживать понимание того, какие идентификаторы не подчиняются обычным правилам области видимости.— Тоби Спейт