• Locked Locked
  • Replies 6 replies
  • Subscribers 80 subscribers
  • Views 636 views
  • Users 0 members are here
Support feedback
Options
Options
Related

This thread has been locked.

If you have a related question, please click the "Ask a related question" button in the top right corner. The newly created question will be automatically linked to this question.

Improving the header files - rename ambiguous defines

Bernhard Weller
Genius 4925 points
Other Parts Discussed in Thread: MSP430F2122

So I recently ran into great trouble with a header file for the MSP430, I think it's in almost every header which actually uses calibration values for DCO. In my specific project I used a MSP430F2122.

So what my problem was (but the root still remains), I tried to set the DCO to it's calibrated 1MHz frequency. An easy task. Just set BCSCTL1 and DCOCTL accordingly like:

BCSCTL1 = CALBC1_1MHZ;
DCOCTL = CALDCO_1MHZ;

Now if you are using the auto-completion feature of Code Composer Studio v4 you will notice (or not as in my case) that you'll most likely use this:

BCSCTL1 = CAL_BC1_1MHZ;
DCOCTL = CAL_DCO_1MHZ;

Now this is totally wrong. If you take a look in the header you will see:

#define CAL_DCO_1MHZ (0x0006) /* Index for DCOCTL Calibration Data for 1MHz */
 SFR_8BIT(CALDCO_1MHZ); /* DCOCTL Calibration Data for 1MHz */

So the meaning of them is quite different. But the name of both won't tell us. In the special case of 1MHz the offset values give roughly a frequency of 1MHz, which makes debugging even harder. I used this for a serial communication, and the bytecount was correct but some bits had randomly errors. As I replaced the MSP because I thought it was damaged during soldering, the next one worked correctly.

I soldered another four boards and well they all didn't work. Handsoldering QFN parts is a bit tricky but I never failed four times in a row and not in such a way (as it's half working). So after scoping the serial transmission I found out it took too long, so the frequency was off. I investigated and found out about these two defines (well more or less defines) which had basically the same name but a different meaning.

 

So what I really would like to see is a clear difference in naming defines, I don't care about long names, just make it as clear as possible what this thing defines. Call it CAL_DCO_1MHZ_FLASH_INDEX or something like that, so that it's obviously the wrong thing to put into the control register. I don't want to imagine a product which actually writes the offset into the registers and works half of the time.

  • Guru 227245 points

    Nobody replied to this, but I think you're right. This kind of naming confusion can lead to major trouble.

    Normally (with the other MSP registers), this isn't a problem, as you cannot write to an rvalue (a number such as the offset). So if you try to assign a value to CAL_DCO_1MHz, the compiler will complain.
    But in case of the calibration values, you're usually reading them, and the compiler will acceppt an lvalue (such as a register/memory location) as well as an rvalue (the offset) when writing it into the clock registers.

    On mspgcc, the rvalues (offsets, register addresses) usually end with an underscore, while the register itself doesn't have one. The compiler will still not see anything wrong using both versions, but at least the programmer will see that a value with trailing underscore is a constant, not a register.

    Another cause of confusion is the naming of multi-bit register settings.. There is e.g. VAL0 and VAL1, which are bit values describing bit 0 and bit 1 of the VAL setting inside a register, while there is also VAL_0, VAL_1, VAL_2 and VAL_3 which are actually the four possible combinations of the two bits. If you confuse one with the other, the outcome is unpredictable.

    On the bottom line, you'll have to learn the correct names and double-check what you wrote. The fact that the compiler doesn't throw an error does not mean your program is correct :)
    Having some strict rules for the naming would help though.

  • in reply to Jens-Michael Gross
    Guru 58965 points

    There are lots of names with single _ and others with double __. Depeding on the character set, they may be difficult to differentiate. One mans "naming convention" can be another's trap.

  • in reply to Jens-Michael Gross
    Genius 4925 points

    Well I'm not a fan of a trailing underscore, it looks like something was forgotten (underscore being a replacement for a space). So I'd much prefer names which clearly express what the underlying thing means.

    The few added bytes here and there pay off very well in respect to code quality.

    And yes the calibration values were only an example which just happened to make my life harder, but as Jens-Michael Gross pointed out there are others which can also be misleading as well. VAL0 and VAL1 could be named VAL_0_BIT and VAL_1_BIT, which should be clear enough.

    And well of course everyone has to learn the correct names and stuff, but these things can happen to everyone being a bit distracted or having a bad day, but even on a bad day I would have seen that writing the offset into the register is definitely the wrong thing to do.

     

    I'm also not a big fan of double underscores, but I don't have any good suggestion for getting rid of these. In most cases they lead to a compiler error if you did it wrong, so it's not that big of a problem for me personally.

  • in reply to Bernhard Weller
    Guru 227245 points

    One problem is that C itself adds a leading underscore to global symbols under some circumstances. Which can lead to confusion.

    I agree than trailing underscores look a bit like something is missing. Anyway, I got used to it that with a trailing underscore, it is a value, without, it is a variable located at the address with this value.

    I remember wehn programming for GEOS, we had defines such as MSG_GEN_VALUE_SET_INTEGER_VALUE (a simple integer message number). There it was almost 100% clear what this is and where it belongs to, even if you didn't know this message at all before. And the Borland C++ compiler used 32 significant letters (instead of the 16 of the microsoft compiler).

    But who wants to write such long names these days? Remember, TI uses CCR0 instead of (or in addition to) TACCR0 just to make the names a bit shorter :)

  • in reply to Jens-Michael Gross
    Genius 4925 points

    Who wants to write such long names? Well certainly nobody wants to write them. But my counter-question is: Who actually writes any of the defines by hand?

    Code Composer Studio, or (I hope) any other specialized IDE will have an auto-completion feature. I use it very often and basically it doesn't matter how long the define is. Type a few beginning letters, hit CTRL + Space and you are presented with a list of possible defines, functions or whatever suitable.

    Well yeah I use some small editors like Notepad++ as well, which don't have an auto-completion feature integrated, but I won't program a whole software using these tools. So basically the only negative thing about long define names is that your program can get a bit hard to read in some cases if you put a lot of defines in one line (e.g. in configuration registers).

    I don't know if the register CCR0 is there to shorten the name of TACCR0 or to get consistency between actual programming and documentation (if in documentation it is referred to  as CCR0 it should be possible to use this in the program as well)

  • in reply to Bernhard Weller
    Guru 227245 points

    Bernhard Weller said:
    Who wants to write such long names? Well certainly nobody wants to write them. But my counter-question is: Who actually writes any of the defines by hand?


    What are the defines good for if they aren't used? It' snot that you write the defines by hand, it's that you have to write the names in your code. Which happens even more often. If you write a define more often than you use it, then there's something wrong (except if you don't use it at all) :)

    The reason for these long names was that the name identifies the role of this define. In my example (what was it? I can't see the older messages while replying - stupid forum software!) it was a message define (there were ATTR and HINT defines as well) for a GenericObejctTree object of type ValueObject and was telling this object to set its value (MSG_GEN_VALUE_SET_VALUE). If you have thousands of possible defines and no today high-end computer (when this was made, a 486 was the bleeding edge technology).

    And guess what - it was faster to find (or memorize) your defines or library function names back then than it is now with a multi-GHz multi-GB PC with a fully integrated IDE such as VisualStudio. "telling" names are always better than cryptic acronyms. And since we all are human, a bit redundancy isn't that bad. Else you go like with telephone numbers: if every possible number has an endpoint, you won't notice that you accidentally switched numbers until you got the wrong one on the other side. That's why companies try to get 'telling' numbers.

    Bernhard Weller said:
    I don't know if the register CCR0 is there to shorten the name of TACCR0 or to get consistency between actual programming and documentation (if in documentation it is referred to  as CCR0 it should be possible to use this in the program as well)

    Indeed, but rather than using different defines for the same thing, the documentation should be changed to match the 'proper' defines.
    One can argue about that. Surely, CCR0 is more generic as it may refer to any timer's CCR0 register. And if this is clear, people should be aware that it needs to be prefixed by the actual timer used. But there are many people who don't read the docs. (if they would, half of the posts here in the forum would vanish). They just look at the examples (which were written in the stone age) and try to use them on the most recent MSP and wonder why this won't work. And instead of educating them, TI has chosen the way of making it a wide paved road, and when they come to the point of madifying the examples, they hit a brick wal at the end of this road with a small door in it.

    Makes entry to the MSP easy, and then frustration explodes. I doubt this is a smart move at the end.

**Attention** This is a public forum