"If we consider the scale of spiritual values ​​in descending order, there are
Things In The Order Of Things Exist
Things Unpleasant, But Basically Permissible, and Exist
Things that tolerate in no way impossible. " - Midianin

In the words of the notorious classic "I can not be silent."
Recently I looked at the source texts on the site of a fairly well-known manufacturer and saw the following code

*(unsigned int *)0xf80ff000 &= 0xffffefff; 

Not hoping that these notes will be read in the distant "India" (see note below) (one gets the feeling that they do not know how to read), I would nevertheless like to warn young engineers not to do so.

A note from today - I sketched this post more than a month ago, all hands did not reach to bring it to an acceptable form, so I can’t specify the source, well, yes Inet will help you - there is a little less such code than a lot.

There is a sea of ​​literature in which it is written how one can and should do (well, so who reads it, there are a lot of letters), I highly recommend the MISRA standards, but such software constructs are not even considered there, since they lie on the other side of the border that separates good from evil But, since such constructions are still encountered, and children can see them (and Ineta’s correct censorship is still not established), you will probably have to explain once again why this cannot be done.

First of all, let us understand what our distant Indian colleagues wanted to make this code fragment (the author is not a chauvinist at all and is ready to admit that the code was written by Chinese, American or Russian programmers, just the Hindu code became a common noun). Obviously, they wanted to reset bit number 12 (counting from 0) in the word with the hexadecimal address f80ff000. Most likely, there is a register of some external device, and, most likely, they reset the readiness bit, which, however, is absolutely unimportant. So, in itself, the operation is not so terrible to push the author to a lengthy post, and, nevertheless, we will begin the debriefing.
')
First of all, in one line 2 magic constants were used at once - for the address and for the data. It is hard for me to believe that nowhere else in the program will there be a single reference to this register (the way it is, such lines can be seen further on the source code) and therefore the use of the magic constant does not have the slightest justification. As you know, #define was invented precisely for this purpose, and although lately I have seen quite a few convincing materials about the need to use constants from the enum list instead of it, even the most unfortunate use of define is better than what we see.

Further, the data is used not just in the form of a magic constant, but it is clearly a transformed constant, that is, an inversion from a bitmask. The only argument that I could invent in favor of such a decision is to reduce the compilation time, but this assumption seems to me so vanishingly insignificant as to the reason that it is given solely for theoretical purposes (well, to emphasize the author’s objectivity and his condescension to human weakness).

Well, yes, I forgot one more argument of constant supporters - named constants clutter up the compiler name table and increase the requirements for RAM size at compile time. I don’t laugh, I really heard such an argument from a kind of sane person, then it turned out that it was subtle trolling (but a dozen unpleasant seconds brought it to me - could I SO IN wrong person).

At this point, we stop trying to justify the authors of such a code (yes, I didn’t really try) and by the method of successive approximations we go to the normal code.

 #define CsrReg *(unsigned int *)0xf80ff000 //  , . 127 #define CsrRegReady 0x1000 //   CsrReg &= ~CsrRegReady; 

already looks better, and we are not even in the middle of the road, although some will stop there. What is wrong with this snippet? First of all, the direct inversion operation, if you are a happy person, you will never miss it when writing. I'm not so happy (maybe not as attentive and as collected as you are), so I used to miss the tilde, and then run around the code in search of a place of error.

Therefore, our macros are all (well, or inline functions, if you switched to pluses)

 #define RegBitClr(REG,MASK) (REG) &= ~(MASK) RegBitClr(CsrReg,CsrRegReady); 

(I know that the names of the macros should be written in capital letters, and even understand the arguments in favor of this statement, but I do not accept them - I am an artist, I see that). In order to understand what is wrong here, it would be nice to see the generated code with optimization turned on for the next fragment.

  RegBitClr(CsrReg,CsrRegReady); RegBitClr(CsrReg,~CsrRegReady); //     

I will not give it, you just have to take my word for it that the first line is ignored, and the second generates a code equivalent to the expression CSrReg = 0. Indeed, from the point of view of the compiler, in the first line we dropped all the bits except 12, and in the second we dropped it, that is, there would be zero.

But in reality, this is absolutely not obvious, since we are dealing with a register, that is, we need a direct indication to the compiler of the changeable nature of the variable and we get

 #define CsrAdr (unsigned int *)0xf80ff000 volatile unsigned int * const CsrReg = CsrAdr; #define RegBitClr(REG,MASK) *(REG) &= ~(MASK) RegBitClr(CsrReg,CsrRegReady); 

Everything is almost good here, note that in this variant we can no longer write

  RegBitClr(CsrRegReady,CsrReg); 

because the compiler curses us. Of course, we don’t have to write like that, this is an incorrect expression, but this post is intended for normal people who make mistakes, programming demigods who never make mistakes can generally do what they want and not think about such boring things as good style. and type checking by the compiler. ("Only mediocrity needs order, Genius rules over Chaos.")

What remained not too successful? The ability to write something like

  RegBitClr(CsrReg,3); 

, we agreed that we should avoid magic constants, and it would be better if the compiler pulled us up, but, unfortunately, this is unattainable in C, remember one of the drawbacks of macros in relation to functions - they do not check the parameters. We cannot switch to a real function, because inline is not a C compiler directive (well, at least, this is in my version of IAR, although I’ve won it like with the help of pragmas — crutches, everywhere crutches), but call the real function for resetting a bit would be very expensive, otherwise

 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); //    

would be an almost perfect solution.

True, you can not write and

  RegBitClrf(CsrReg,CsrRegReady | CsrRegDone); //     RegBitClrf(CsrReg,(enum CsrRegBits)(CsrRegReady | CsrRegDone)); //     

Of course, the last line allows you to create unacceptable expressions, but at least you have clearly indicated that you are familiar with the length of the rope. The only thing I would recommend for such a solution, at least for IAR, is to check the box “Treat all warnings as errors” to make the control tough.

Well and the conclusion - we meanwhile considered only the one-task uninterrupted program. Imagine that you need to take into account the possibility of interruptions, I will do this by adding two lines to the text of a macro or function, but it’s hard for me to imagine what the authors of the original fragment will do.

An important addition is in the comments one of the users of Habr, namely pwl, offered an absolutely fascinating solution to the problem of type checking, namely, adding a dummy function to the macro with macro arguments, and the compiler checks the type and issues the varning, but the call to the function itself is suppressed because its the result participates in a constant logical expression. That is, the wolves are fed (we checked the type) and the sheep are intact (the resulting code has not grown). Many thanks again, this is just what I call a fruitful discussion, it’s just a pity that I didn’t think of it myself. This is the perfect solution.

 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); //