-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/recomendations from google doc #84
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -294,6 +294,31 @@ if (cond1) { | |||||
|
||||||
Часто бывает что устройство имеет несколько входов/выходов/шин/светодиодов/кнопок итд, абсолютно всегда все что потенциально может быть больше чем в одном экземпляре должно обрабатываться через индексы и циклы. | ||||||
|
||||||
Плохой пример: | ||||||
```C | ||||||
#define INPUT_1 { GPIOA, 10 } | ||||||
#define INPUT_2 { GPIOA, 11 } | ||||||
|
||||||
... | ||||||
|
||||||
gpio_init(INPUT_1); | ||||||
gpio_init(INPUT_2); | ||||||
``` | ||||||
|
||||||
В таком виде трудно вносить изменения, легко допустить ошибку, поправить одно и забыть другое. | ||||||
|
||||||
Лушче написать так: | ||||||
```C | ||||||
#define INPUTS_NUMBER 2 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. В принципе можно и без этого обойтись, а использовать ARRAY_SIZE |
||||||
#define INPUTS { { GPIOA, 10 }, { GPIOA, 11 } } | ||||||
|
||||||
... | ||||||
|
||||||
for (int i = 0; i < INPUTS_NUMBER; i++) { | ||||||
gpio_init(INPUTS[i]); | ||||||
} | ||||||
``` | ||||||
|
||||||
## Исправление ошибок в master | ||||||
|
||||||
Ранее допущенные ошибки форматирования исправляются в отдельной ветке с PR согласно кодстайлу. | ||||||
|
@@ -395,3 +420,100 @@ enum w1_protocol_transaction { | |||||
W1_PROTOCOL_TRANSACTION_RECEIVE | ||||||
} | ||||||
``` | ||||||
# switch case | ||||||
Старайтесь не использовать switch case. Если вы делаете несколько конструкций switch case по одной и той же переменной то это признак того что вам нужно использовать полиморфизм. Объедините в структуру все сущности, зависящие от этой переменной. Создайте массив таких структур и используйте переменную в качестве индекса для доступа к текущей структуре. | ||||||
|
||||||
Например вместо: | ||||||
```C | ||||||
void input_handler(unsigned input) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Мне кажется, пример неудачный. Надо что-то посложнее. Тут я вижу из плюсов только то, что код быстрее на микросекунду будет работать, а из минусов: надо проверять индекс на выход на пределы массива; если искать функции просто поиском, они нигде не вызываются, а только лежат в каком-то массиве, который ещё надо понять как работает; ну и скорее всего придется делать обертки, т.к. вряд ли окажется, что все обработчики одного типа. В общем, имхо в самом switch-case нет ничего плохого. И он много где используется у нас. Надо подобрать более подходящий пример, где его лучше не использовать |
||||||
switch (input) { | ||||||
case SIGNAL_IRQ: | ||||||
signal_irq_handler(); | ||||||
break; | ||||||
case SIGNAL_ERROR: | ||||||
signal_error_handler(); | ||||||
break; | ||||||
case SIGNAL_READY: | ||||||
signal_ready_handler(); | ||||||
break; | ||||||
default: | ||||||
break; | ||||||
} | ||||||
} | ||||||
``` | ||||||
|
||||||
Лучше использовать: | ||||||
```C | ||||||
|
||||||
void input_handler(unsigned input) { | ||||||
static const void (*handlers[])(void) = { | ||||||
[SIGNAL_IRQ] = signal_irq_handler, | ||||||
[SIGNAL_ERROR] = signal_error_handler, | ||||||
[SIGNAL_READY] = signal_ready_handler, | ||||||
}; | ||||||
handlers[input](); | ||||||
} | ||||||
``` | ||||||
|
||||||
Такой код компактнее, быстрее выполняется. Также он обеспечивает консистентность, так как по этим же индексам у нас наверняка будет описан список портов GPIO на который подключены эти сигналы, итд. | ||||||
|
||||||
# Не плодите макросы конфигурации | ||||||
Часто макросами в коде выбирается какая переферия используется в данном варианте, вариантов обычно не много, а переферии наоборот много. Вместо того чтобы прописывать макрос на каждый регистр или значение используйте структуры. | ||||||
|
||||||
Вместо такого: | ||||||
```C | ||||||
#define MODBUS_HW_USART USART1 | ||||||
#define MODBUS_HW_USART_IRQn USART1_IRQn | ||||||
#define MODBUS_HW_DMA_TX_CH DMA1_Channel2 | ||||||
``` | ||||||
|
||||||
Лучше написать так: | ||||||
```C | ||||||
struct modbus_hw { | ||||||
USART_TypeDef *usart; | ||||||
IRQn_Type irqn; | ||||||
DMA_Channel_TypeDef *dma_tx_ch; | ||||||
}; | ||||||
|
||||||
#define MODBUS_USART_HW { \ | ||||||
.usart = USART1, \ | ||||||
.irqn = USART1_IRQn, \ | ||||||
.dma_tx_ch = DMA1_Channel2 \ | ||||||
} | ||||||
|
||||||
``` | ||||||
|
||||||
# Переиспользование | ||||||
Нужно использовать код повторно. Не нужно писать код если он уже написан. Если чтото существующее подходит не нужно делать чтото новое. | ||||||
Чтобы не оставлять ошибки в дубликатах, экономить силы программиста и прочее. Читать подробнее в [Википедии](https://ru.wikipedia.org/wiki/%D0%9F%D0%BE%D0%B2%D1%82%D0%BE%D1%80%D0%BD%D0%BE%D0%B5_%D0%B8%D1%81%D0%BF%D0%BE%D0%BB%D1%8C%D0%B7%D0%BE%D0%B2%D0%B0%D0%BD%D0%B8%D0%B5_%D0%BA%D0%BE%D0%B4%D0%B0). | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Может юникодную ссылку вставить? |
||||||
Если есть 2 одинаковые строчки, подумайте, не оформить ли это в функцию, библиотеку или использовать в уже существующую. | ||||||
Для использования кода с другого устройства - сделайте его библиотекой. | ||||||
|
||||||
|
||||||
|
||||||
# Поллинг | ||||||
Не должно быть периодически вызываемых функций с проверкой флагов. Поллинг это плохо. Тратится процессорное время и память для флагов которые проверяются. Также не понятно кто этот флаг ставит, кто его проверяет. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ну тоже не согласен. Вот в WBIO поллинг. Написано категорично, переделываем? А отказаться от поллинга не всегда возможно. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. было бы лучше если бы WBIO был написан асинхронно. если бы входы генерили прерывания и поддерживали консистентность данных в карте регистров, которые ты забирал бы в обработчике i2c не опрашивая ноги. видно что в этом потенциальная проблема. выходы пришлось выносить в бэкграунд а со входами ты так не сможешь. архитектура без поллинга всегда лучше. то что мы используем поллинг это наши привычки и лень. |
||||||
|
||||||
У любого кода есть причина выполнения, вот в обработке причины и нужно вызывать выполнение кода, или запланировать его выполнение. | ||||||
Используйте инструменты организации кода - корутины и таймменеджер. В прерываниях планируйте задачу с обработкой в бэкграунде. Не используйте флаги. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ну сам таймменеджер внутри флаги поллит же |
||||||
|
||||||
# Не пишите лишний код, не выполняйте не нужные действия в коде | ||||||
Пример: если файл с настройками не найден, прошивка использует дефолтные настройки. Не нужно сохранять файл с дефолтными настройками. Файл нужно записывать только если пользователь чтото поменял. | ||||||
Чтение файла с дефолтными настройками ничем не отличается от отсутствия файла. По этому можно избавиться от лишней операции записи, расхода места иизноса флеш памяти, кода который эту запись делает. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
# Не нужно делать в run time все что можно сделать в compile time | ||||||
Все вычисления констант, генерацию макросов итд, то что не меняется в ходе работы программы нужно вычислять препроцессором на этапе сборки и записывать readonly константами: ресурсы ограничены, и их нужно экономить! Пусть мощный компьютер подготовит все необходимые данные. | ||||||
По этой же причине следует выделять память статически и не использовать кучу. | ||||||
|
||||||
# Не нужно делать в контексте прерывания все, что можно сделать в контексте background | ||||||
Синхронизация асинхронных процессов это источник проблем. У нас есть разные инструменты для организации такой архитектуры. Тайм менеджер, корутины. Мы всегда должны знать что выполняется в прерываниях ВО ВСЕЙ ПРОШИВКЕ, чтобы быть уверенными что там нет ничего лишнего. | ||||||
Например: в прерывании вызывать только планирование таски, а не само её выполнение. | ||||||
|
||||||
# Не усложняйте код | ||||||
Не нужно рассчитывать на тонкости стандартов языка. Не нужно заворачивать сложные конструкции. Код должен прочитать школьник который осилил только первую главу книжки по Си. | ||||||
Подумайте можно ли сделать ваш код проще ? Часто можно выделить функции, чтото перегруппировать, распутать. | ||||||
Помните про бритву Оккама. | ||||||
|
||||||
# Делайте сразу хорошо если понимаете как | ||||||
Не нужно писать TODO и FIXME. Если в код можно сделать компактнее, алгоритм быстрее, и вы знаете как, то сделайте сразу хорошо. Не надо лениться и аргументировать почему и так пойдет. | ||||||
Плохо аргументировать фразами "жалко чтоли ? там не много". Такая аргументация не учитывает всю систему целиком, все возможные комбинации и многообразие кейсов применения. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Файлик называется embedded_c.en.md, а контент на русском.