«Если рассматривать шкалу духовных ценностей по нисходящей, существуют
Вещи В Порядке Вещей, существуют
Вещи Неприятные, Но В Принципе Допустимые, и существуют
Вещи, Которые Терпеть Никак Нельзя». — Мидянин


Как говорил небезызвестный классик «Не могу молчать».
Недавно смотрел исходные тексты на сайте довольно-таки известного производителя и увидел следующий код
*(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); // здесь тильда поставлена специально
Я его приводить не буду, Вам остается только поверить мне на слово, что первая строка игнорируется, а вторая порождает код, эквивалентный выражению CSrReg=0. И действительно, с точки зрения компилятора, в первой строке мы сбросили все биты, кроме 12, а во второй сбросили и его, то есть будет ноль.

Но на самом то деле это совершенно не очевидно, поскольку мы имеем дело с регистром, то есть нам нужно прямое указание компилятору на изменчивый характер переменой и мы получаем
#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);
, мы ведь договорились, что должны избегать магических констант, и лучше бы, если бы компилятор нас одергивал, но, к сожалению, в рамках С это недостижимо, вспомним один из недостатков макросов по отношению к функциям — они не проверяют параметры. Перейти же к настоящим функция мы не можем, поскольку inline не является в C директивой компилятора (ну, по крайней мере, так в моей версии IAR, хотя я вроде это победил при помощи прагмы — костыли, повсюду костыли), а вызывать настоящую функцию для сброса бита будет весьма накладно, иначе бы
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)); // а вот так можно
 
Конечно, последняя строка позволяет создать и неприемлемые выражения, но, по крайней мере, Вы явно указали, что ознакомлены с длиной веревки. Единственно, что порекомендую для такого решения, по крайней мере для IAR, поставить флажок против «Treat all warnings as errors» чтобы сделать контроль жестким.

Ну и заключение — мы пока что рассматривали только однозадачную непрерываемую программу. Представим, что Вам надо учесть возможность прерываний, я-то это сделаю путем добавления двух строчек в текст макроса либо функции, а вот что будут делать авторы исходного фрагмента, мне даже и представить трудно.

Важное дополнение — в комментариях один из пользователей Хабра, а именно 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)


  1. yatagarasu
    03.12.2015 14:48
    +2

    Недавно смотрел исходные тексты на сайте довольно-таки известного производителя и увидел следующий код

    Мне трудно поверить, что больше нигде в программе не будет ни одного обращения к этому регистру

    Пока была показана лишь одна строка подобного кода, а весь юзкейс построен лишь на некотором предположении, то данную статью можно воспринимать и как самоучитель «как написать 10 строк кода, вместо одной»


    1. GarryC
      03.12.2015 15:03
      +15

      Если полученный 10 строк кода более надежны, понятны и, внимание, не производят объектный код бОльшей длины, то я голосую за 10 строк кода. После того, как Вы полчаса будете смотреть на свой код через пол-года после написания с мучительными мыслями «А что я имел в виду, когда переворачивал этот бит в этой одной строке?», Вы тоже склонитесь к 10 строкам, хотя кто знает…


      1. velvetcat
        03.12.2015 16:10
        -4

        После того, как Вы полчаса будете смотреть на свой код через пол-года после написания с мучительными мыслями «А что я имел в виду, когда переворачивал этот бит в этой одной строке?

        Не надо драматизировать, это далеко не пример ужасного кода.


      1. yatagarasu
        03.12.2015 16:44
        -3

        Зависист от спецификации на устройство, если там всё расписано в понятиях *(unsigned int *)0xf80ff000 &= 0xffffefff; то и писать лучше так, а не придумывать свои имена. Да и интерфейс из одной комманды «сброс бита по адресу» выглядит очень странным и не полным.
        И про магические числа тут притянуто немного за уши. Для человека работающего с железом 0xf80ff000 и CsrReg будут одинаковыми уникальными именами некого регистра. Числовой формат возможно содержит ещё больше информации, например это регистр из группы 0хf8xxxxxxxxx
        А вопросы а что я имел ввиду переворачивая бит — возникнут в обеих случаях, так что если и вводить абстракцию то на каком-то более высоком уровне.


  1. IronHead
    03.12.2015 15:35
    +1

    Статья высосана из пальца.
    Во первых: все знают, что magic numbers это плохо.
    Во вторых: А вдруг эта ваша строчка была в исходнике какой нибудь библиотеки в контексте:
    void reset_uartrx_interrupt()
    {
    *(unsigned int *)0xf80ff000 &= 0xffffefff;
    }
    Тогда ваши 10 строк кода ничего не дадут, кроме увеличения количества кода.


    1. GarryC
      03.12.2015 17:32
      +4

      Ну вообще то я показал, что дадут эти десять строк — возможность быстрой модификации (в целях переноса в том числе), автоматическую проверку компилятором (что устраняет возможные ошибки), причем за бесплатно (то есть даром) — удивлен, что Вы этого не увидели. Как я и сказал в тексте этот пост не для «полубогов в программировании», которые не делают ошибок НИКОГДА.
      А вообще то забавно получилось 1) все знают, что это плохо 2) но это плохо не всегда?


      1. IronHead
        03.12.2015 17:42
        +1

        Приведите полный кусок кода с этой конструкцией, дабы оценить необходимость в дополнительных строчках.


  1. olekl
    03.12.2015 16:19
    +2

    «а вот что будут делать авторы исхлжного фрагмента, мне даже и представить трудно» — добавит куда-нибудь «профилактический ресет» всей системы. Или захардкодит несколько попыток обращения последовательно :)

    А если серьезно — практически для каждого микроконтроллера уже ведь есть неплохо написанные заголовки, где все регистры объявлены так, чтоб работало… Хотя, может кому-то религия не позволяет их использовать…


  1. olekl
    03.12.2015 16:22

    «а вызывать настоящую функцию для сброса бита будет весьма накладно» — вот как раз недавно писал про mbed, в котором почти сброс бита (изменение GPIO пина с IN на OUT) на контроллере с частотой 48 МГц занимал 13 микросекунд…


    1. GarryC
      03.12.2015 17:34

      Ну там все-таки, наверное, настройка всех параметров пина (вход-выход, функция, подтяжки, скорости нарастания, фильтры и т.д.), а не только направления?


      1. olekl
        03.12.2015 18:05

        Не-а, вызов одной функции типа pin.input(); Просто там С++ фреймворк аппаратно-независимый.


        1. 0xd34df00d
          05.12.2015 22:13

          Вызов съедал под 624 такта что ли? Сложно поверить.


  1. Konachan700
    03.12.2015 16:48

    Видел такой стиль в исходниках загрузчика одного древнего арма. Когда увидел впервые было недоумение: зачем они так делают, нечитаемо же! Но потом, ковыряя код, дошло — это местами очень удобно для мелких правок. В открытом даташите вбиваем адрес и нам сразу показывают что это, оттуда же можно адрес скопипастить при дописывании функционала. Минус время на поиск в даташите. В случае с #define надо либо в IDE код смотреть, что не всегда возможно, либо искать нужную константу по пачке громадных заголовочных файлов. И одна константа может быть объявлена там сотню раз с разными адресами через #ifdef для разных процов. То есть правка пары-тройки регистров превращается в весьма затейливый трудоёмкий квест, если все по правилам делать, а код этот все-равно никто почти не увидит. Можно, безусловно, свой дублирующий #define сделать, но зачем? Так что не настолько однозначно плох способ.


    1. iliasam
      03.12.2015 18:12
      +1

      Это хорошо, если в даташите для каждого регистра прямо указан его адрес. В STM32, например, обычно указывают базовый адрес периферийного модуля, а в описаниях регистров указывают только смещения от него.


  1. Indemsys
    03.12.2015 17:13
    +2

    Эт точно.
    Выполнение сброса бита в таком виде

    *(unsigned int *)0xf80ff000 &= 0xffffefff;
    это скорее всего копипаста из какого-то низкоуровневого мануала.
    Это верный признак, что здесь код никто кроме кодера не проверял, а может и не отлаживал.


    1. GarryC
      03.12.2015 17:35
      +2

      Хм, а такой вариант мне даже в голову не пришел. А ведь вполне могла быть и копипаста, спасибо за подсказку.


  1. ababo
    03.12.2015 18:59
    -2

    Перенесите в habrahabr.ru.


    1. MaximChistov
      03.12.2015 19:20
      +6

      куда?
      хаб Программирование микроконтроллеров администрация оттуда удалила


  1. pwl
    03.12.2015 23:00
    +1

    RegBitClr(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);
    }
    


    1. MaximChistov
      04.12.2015 00:10

      неплохо :)


    1. GarryC
      04.12.2015 11:30

      А IAR мне бросает варнинг, наверное, дело в настройках, в проектах IAR их много и далеко не все я трогал. Но в данном случае я целиком и полнностью на стороне IAR — такое неявное приведение должно быть помечено, как источник возможной ошибки, иначе возникает вопрос, а нафиг нам вообще тогда enum? Кстати, раньше я думал что IAR юзает gcc (непонятно, почему такая идея была у меня в голове), но после ряда экспериментов убедился, что это не так.

      А вот по поводу Вашей проверочной функции — большое спасибо. Это же ассерт времени компиляции, то, что надо. И как здорово с подавлением его исполнения через логическое выражение — я бы никогда не додумался, еще раз спасибо.
      Я подозревал, что есть люди, знающие С лучше меня, но сейчас я в этом убедился и это нисколько не сарказм. Сейчас добавлю это решение к основному посту, со ссылкой на Вас, если не возражаете.


      1. pwl
        04.12.2015 20:28

        Всегда рад помочь :)
        Все эти трюки придуманы еще в прошлом веке, поэтому врядли я могу претендовать на авторство :)


  1. tangro
    04.12.2015 12:00

    А можно было бы просто написать юнит-тест на вызов этой функции, который бы проверял, что что-то выставилось или не выставилось. Занял бы две строки и проверил бы не менее надёжно (плюс ещё бы и подсказал читателю кода как функцию вызывать и чего от неё ожидать).


    1. GarryC
      04.12.2015 14:39

      Ну, вообще то, я про С писал.
      И вообще я не уверен, что в embedded система юнит-тестов — хорошая идея.


  1. alexpic
    07.12.2015 09:21

    Вам не думали о том, что этот кусок кода мог быть сгенерирован автоматизированными средствами? Обидно за индусов :)