«Если рассматривать шкалу духовных ценностей по нисходящей, существуют
Вещи В Порядке Вещей, существуют
Вещи Неприятные, Но В Принципе Допустимые, и существуют
Вещи, Которые Терпеть Никак Нельзя». — Мидянин
Как говорил небезызвестный классик «Не могу молчать».
Недавно смотрел исходные тексты на сайте довольно-таки известного производителя и увидел следующий код
*(unsigned int *)0xf80ff000 &= 0xffffefff;
Примечание от сегодняшнего дня — набросал этот пост больше месяца назад, все руки не доходили довести его до приемлемого вида, так что источник указать не могу, ну да Инет Вам в помощь — подобного кода там чуть меньше, чем очень много.
Существует море литературы, в которой написано, как делать можно и нужно (ну так кто же ее читает, там много букв), я настоятельно рекомендую стандарты MISRA, но подобные программные конструкции там даже не рассматриваются, поскольку они лежат по ту сторону границы, отделяющую добро от зла. Но, поскольку такие конструкции все еще встречаются, и их могут увидеть дети (а правильная цензура Инета все еще не налажена), наверное, придется еще раз объяснить, почему так делать нельзя.
Для начала поймем, что же хотели сделать этим фрагментом кода наши далекие индийские коллеги (автор нисколько не шовинист и готов допустить, что код написан китайскими, американскими либо русскими программистами, просто индусский код стал нарицательным термином). Очевидно, что они хотели сбросить бит номер 12 (считая с 0) в слове с шестнадцатеричным адресом f80ff000. Скорее всего, там расположен регистр какого-то внешнего устройства, и, скорее всего, они сбрасывают бит готовности, что, впрочем, совершенно неважно. Итак, сама по себе операция не настолько ужасна, чтобы подвигнуть на автора на пространный пост, и, тем не менее, начнем разбор полетов.
Прежде всего, в одной строке использовано сразу 2 магические константы — для адреса и для данных. Мне трудно поверить, что больше нигде в программе не будет ни одного обращения к этому регистру (так оно и есть, подобные строки можно увидеть далее по исходному коду) и поэтому использование магической константы не имеет ни малейшего оправдания. Как известно, #define именно для этого и придуман, и хотя в последнее время я видел немало убедительных материалов о необходимости использования констант из списка enum вместо него, даже самое неудачное применение дефайна лучше, чем то, что мы видим.
Далее, данные использованы не просто в виде магической константы, но это явно константа преобразованная, то есть инверсия от битовой маски. Единственный аргумент, который я мог бы изобрести в пользу подобного решения, это снижение времени компиляции, но мне данное предположение представляется настолько исчезающе ничтожной мотивировкой, что приведено исключительно в теоретических целях (ну и чтобы подчеркнуть объективность автора и его снисходительность к человеческим слабостям).
А ну да, я забыл еще один аргумент сторонников констант — именованные константы захламляют таблицу имен компилятора и увеличивают требования к размеру оперативной памяти во время компиляции. Я не смеюсь, я действительно слышал такой аргумент от вроде как вменяемого человека, потом выяснилось, что это был тонкий троллинг (но десяток неприятных секунд мне это принесло — неужели я мог НАСТОЛЬКО в человеке ошибаться).
На этом прекращаем попытки оправдать авторов подобного кода (да я не особо-то и пытался) и методом последовательных приближений пойдем к нормальному коду.
#define CsrReg *(unsigned int *)0xf80ff000 // регистр состояний, стр. 127
#define CsrRegReady 0x1000 // бит готовности
CsrReg &= ~CsrRegReady;
Поэтому макросы наше все (ну или inline функции, если Вы перешли на плюсы)
#define RegBitClr(REG,MASK) (REG) &= ~(MASK)
RegBitClr(CsrReg,CsrRegReady);
RegBitClr(CsrReg,CsrRegReady);
RegBitClr(CsrReg,~CsrRegReady); // здесь тильда поставлена специально
Но на самом то деле это совершенно не очевидно, поскольку мы имеем дело с регистром, то есть нам нужно прямое указание компилятору на изменчивый характер переменой и мы получаем
#define CsrAdr (unsigned int *)0xf80ff000
volatile unsigned int * const CsrReg = CsrAdr;
#define RegBitClr(REG,MASK) *(REG) &= ~(MASK)
RegBitClr(CsrReg,CsrRegReady);
RegBitClr(CsrRegReady,CsrReg);
Что осталось не слишком удачным? Возможность написать что-нибудь типа
RegBitClr(CsrReg,3);
typedef volatile unsigned int * const Reg_t;
enum CsrRegBits {CsrRegReady=0x1000,CsrRegDone=0x100};
inline void RegBitClr(Reg_t Reg, const enum CsrBits Mask) {
*Reg &= ~Mask;
};
RegBitClr(CsrReg,CsrRegReady);
RegBitClr(CsrReg,~CsrRegReady); // тут и
RegBitClr(CsrReg,3); // тут предупреждения компилятора
Правда, нельзя написать и
RegBitClrf(CsrReg,CsrRegReady | CsrRegDone); // тут тоже предупреждение компилятора
RegBitClrf(CsrReg,(enum CsrRegBits)(CsrRegReady | CsrRegDone)); // а вот так можно
Ну и заключение — мы пока что рассматривали только однозадачную непрерываемую программу. Представим, что Вам надо учесть возможность прерываний, я-то это сделаю путем добавления двух строчек в текст макроса либо функции, а вот что будут делать авторы исходного фрагмента, мне даже и представить трудно.
Важное дополнение — в комментариях один из пользователей Хабра, а именно pwl предложил совершенно очаровательное решение проблеммы с проверкой типов, а именно добавление в макрос фиктивной функции с аргументами макроса, причем компилятор проверяет тип и выдает варнинг, но вызов самой функции подавлен, так как ее результат учавствует в константном логическом выражении. То есть и волки сыты (мы проверили тип) и овцы целы (результирующий код не вырос). Большое спасибо еще раз, это как раз то, что я называю плодотворным обсуждением, жалко только, что сам до подобного не додумался. Вот это совершенное решение
typedef volatile unsigned int * const Reg_t;
enum CsrRegBits {CsrRegReady=0x1000,CsrRegDone=0x100};
void TestBitClrArg(Reg_t adr, enum CsrRegBit data); // тело функции необязательно, она все равно не вызовется
#define RegBitClr(REG,MASK) (*(REG) &= ~(MASK), 1||TestBitClrArg(REG,BIT))
RegBitClr(CsrReg,CsrRegReady);
RegBitClr(CsrReg,~CsrRegReady); // тут и
RegBitClr(CsrReg,3); // тут предупреждения компилятора
Комментарии (25)
IronHead
03.12.2015 15:35+1Статья высосана из пальца.
Во первых: все знают, что magic numbers это плохо.
Во вторых: А вдруг эта ваша строчка была в исходнике какой нибудь библиотеки в контексте:
void reset_uartrx_interrupt()
{
*(unsigned int *)0xf80ff000 &= 0xffffefff;
}
Тогда ваши 10 строк кода ничего не дадут, кроме увеличения количества кода.GarryC
03.12.2015 17:32+4Ну вообще то я показал, что дадут эти десять строк — возможность быстрой модификации (в целях переноса в том числе), автоматическую проверку компилятором (что устраняет возможные ошибки), причем за бесплатно (то есть даром) — удивлен, что Вы этого не увидели. Как я и сказал в тексте этот пост не для «полубогов в программировании», которые не делают ошибок НИКОГДА.
А вообще то забавно получилось 1) все знают, что это плохо 2) но это плохо не всегда?IronHead
03.12.2015 17:42+1Приведите полный кусок кода с этой конструкцией, дабы оценить необходимость в дополнительных строчках.
olekl
03.12.2015 16:19+2«а вот что будут делать авторы исхлжного фрагмента, мне даже и представить трудно» — добавит куда-нибудь «профилактический ресет» всей системы. Или захардкодит несколько попыток обращения последовательно :)
А если серьезно — практически для каждого микроконтроллера уже ведь есть неплохо написанные заголовки, где все регистры объявлены так, чтоб работало… Хотя, может кому-то религия не позволяет их использовать…
olekl
03.12.2015 16:22«а вызывать настоящую функцию для сброса бита будет весьма накладно» — вот как раз недавно писал про mbed, в котором почти сброс бита (изменение GPIO пина с IN на OUT) на контроллере с частотой 48 МГц занимал 13 микросекунд…
Konachan700
03.12.2015 16:48Видел такой стиль в исходниках загрузчика одного древнего арма. Когда увидел впервые было недоумение: зачем они так делают, нечитаемо же! Но потом, ковыряя код, дошло — это местами очень удобно для мелких правок. В открытом даташите вбиваем адрес и нам сразу показывают что это, оттуда же можно адрес скопипастить при дописывании функционала. Минус время на поиск в даташите. В случае с #define надо либо в IDE код смотреть, что не всегда возможно, либо искать нужную константу по пачке громадных заголовочных файлов. И одна константа может быть объявлена там сотню раз с разными адресами через #ifdef для разных процов. То есть правка пары-тройки регистров превращается в весьма затейливый трудоёмкий квест, если все по правилам делать, а код этот все-равно никто почти не увидит. Можно, безусловно, свой дублирующий #define сделать, но зачем? Так что не настолько однозначно плох способ.
iliasam
03.12.2015 18:12+1Это хорошо, если в даташите для каждого регистра прямо указан его адрес. В STM32, например, обычно указывают базовый адрес периферийного модуля, а в описаниях регистров указывают только смещения от него.
Indemsys
03.12.2015 17:13+2Эт точно.
Выполнение сброса бита в таком виде*(unsigned int *)0xf80ff000 &= 0xffffefff;
это скорее всего копипаста из какого-то низкоуровневого мануала.
Это верный признак, что здесь код никто кроме кодера не проверял, а может и не отлаживал.GarryC
03.12.2015 17:35+2Хм, а такой вариант мне даже в голову не пришел. А ведь вполне могла быть и копипаста, спасибо за подсказку.
ababo
03.12.2015 18:59-2Перенесите в habrahabr.ru.
MaximChistov
03.12.2015 19:20+6куда?
хаб Программирование микроконтроллеров администрация оттуда удалила
pwl
03.12.2015 23:00+1RegBitClr(CsrReg,3); // тут предупреждения компилятора
Какие-то у вас с IAR-ом странные представления о C. Мне не удалось получить warning при неявном приведении int-а в enum, ни в MSVC (с /Wall), ни в gcc (с -W -Wall).
Вот в режиме C++, да, ругаются оба. Но ошибками, а не warning-ами.
Перейти же к настоящим функция мы не можем, поскольку inline не является в C директивой компилятора (ну, по крайней мере, так в моей версии IAR, хотя я вроде это победил при помощи прагмы — костыли, повсюду костыли), а вызывать настоящую функцию для сброса бита будет весьма накладно
Ну если вы уж так сильно желаете странного, можно сделать что-то вроде такого:
enum e {e1}; int fake_fnc(int* a, enum e v); // тела для функции не нужно, т.к. она никогда не вызывается #define DO_STUFF(a,v) (*(a)=(v),1||fake_fnc(a,v)) void main(void) { int z; DO_STUFF(&z, 1); // тут в случае C++ у нас будет ошибка DO_STUFF(&z, e1); }GarryC
04.12.2015 11:30А IAR мне бросает варнинг, наверное, дело в настройках, в проектах IAR их много и далеко не все я трогал. Но в данном случае я целиком и полнностью на стороне IAR — такое неявное приведение должно быть помечено, как источник возможной ошибки, иначе возникает вопрос, а нафиг нам вообще тогда enum? Кстати, раньше я думал что IAR юзает gcc (непонятно, почему такая идея была у меня в голове), но после ряда экспериментов убедился, что это не так.
А вот по поводу Вашей проверочной функции — большое спасибо. Это же ассерт времени компиляции, то, что надо. И как здорово с подавлением его исполнения через логическое выражение — я бы никогда не додумался, еще раз спасибо.
Я подозревал, что есть люди, знающие С лучше меня, но сейчас я в этом убедился и это нисколько не сарказм. Сейчас добавлю это решение к основному посту, со ссылкой на Вас, если не возражаете.pwl
04.12.2015 20:28Всегда рад помочь :)
Все эти трюки придуманы еще в прошлом веке, поэтому врядли я могу претендовать на авторство :)
tangro
04.12.2015 12:00А можно было бы просто написать юнит-тест на вызов этой функции, который бы проверял, что что-то выставилось или не выставилось. Занял бы две строки и проверил бы не менее надёжно (плюс ещё бы и подсказал читателю кода как функцию вызывать и чего от неё ожидать).
GarryC
04.12.2015 14:39Ну, вообще то, я про С писал.
И вообще я не уверен, что в embedded система юнит-тестов — хорошая идея.
alexpic
07.12.2015 09:21Вам не думали о том, что этот кусок кода мог быть сгенерирован автоматизированными средствами? Обидно за индусов :)
yatagarasu
Пока была показана лишь одна строка подобного кода, а весь юзкейс построен лишь на некотором предположении, то данную статью можно воспринимать и как самоучитель «как написать 10 строк кода, вместо одной»
GarryC
Если полученный 10 строк кода более надежны, понятны и, внимание, не производят объектный код бОльшей длины, то я голосую за 10 строк кода. После того, как Вы полчаса будете смотреть на свой код через пол-года после написания с мучительными мыслями «А что я имел в виду, когда переворачивал этот бит в этой одной строке?», Вы тоже склонитесь к 10 строкам, хотя кто знает…
velvetcat
Не надо драматизировать, это далеко не пример ужасного кода.
yatagarasu
Зависист от спецификации на устройство, если там всё расписано в понятиях *(unsigned int *)0xf80ff000 &= 0xffffefff; то и писать лучше так, а не придумывать свои имена. Да и интерфейс из одной комманды «сброс бита по адресу» выглядит очень странным и не полным.
И про магические числа тут притянуто немного за уши. Для человека работающего с железом 0xf80ff000 и CsrReg будут одинаковыми уникальными именами некого регистра. Числовой формат возможно содержит ещё больше информации, например это регистр из группы 0хf8xxxxxxxxx
А вопросы а что я имел ввиду переворачивая бит — возникнут в обеих случаях, так что если и вводить абстракцию то на каком-то более высоком уровне.