Cleaning up after EEs



  • Disclaimer: No offense is intended toward the many talented EEs who may or may not be on this forum (fora?). I lived with an EE for three years and he seemed alright. Besides, the things in this post are not that bad, just small errors that I thought were interesting for highlighting the difference between the way the EEs and the rest of us think about programs.

    So I'm the only person on my team whose education was in Computer Science rather than Mechanical Engineering or Electrical Engineering. Since I'm able to bring a different perspective to the software we produce, my manager considers me a valuable member of the team. One of the first things I did at this job was conduct a code review of a C++ program written by a few of the team members. This software runs on a Texas Instruments microcontroller, so space is extremely limited (including ROM space, where the executable code lives).

    One of the first things I noticed was a misunderstanding of compilation units and the behavior of the keyword extern. One file, main_vars.h, contained the following global variable definitions:

    typedef struct Control_Setpoints
    {
      int Message_Type;
      int Message_Length;
      unsigned long long Time_Stamp_Sec;
      int Time_MS;
    
      /* snip 40 more fields */
    
    } Control_Setpoints;
    
    /* snip 8 other structs just as large */

    And in another file, main_extern_vars.h...

    extern struct Control_Setpoints
    {
      int Message_Type;
      int Message_Length;
      unsigned long long Time_Stamp_Sec;
      int Time_MS;
    
      /* snip 40 more fields */
    
    } Control_Setpoints;
    
    /* snip the rest of the structs, also duplicated entirely with the extern keyword added */

    main_vars.h was included in one place and main_extern_vars.h was included everywhere else. Which means that any changes had to be duplicated across these files - which took a while, given the size of the files, and any typos would cause a compilation error. Half the comments for the file were in main_vars.h and the other half were in main_extern_vars.h. I moved the typedefs into one file, included it in the other vars.h files, and removed the superfluous type re-declarations.

    The next disconnect was sort of amusing: the true and false keywords were nowhere to be found in the program, despite the 100+ boolean variables. Assigning to one of them was done with 1s and 0s, as in voltageError = 1; thingComplete = 0; etc. I assume this was done due to the authors thinking of the booleans directly as bits - which is understandable, given that they're EEs - and ignored the abstraction provided for them. I asked about changing them all to true and false and got a nonenthusiastic response, so I left it.

    I also came across the following construct:

    if (GIODINA & 8 == 8)
        voltageAlarm = 1;
    else
        voltageAlarm = 0;
    WTF, I thought. Never mind that GIODINA is a TI-provided variable of a TI-provided type which allows you to simply say GIODINA.bit3 to get the fourth bit. Why not replace that statement with voltageAlarm = GIODINA & 8;, assigning the bit to the boolean value directly? There were dozens of these throughout the codebase. I replaced them all with the corresponding one-liner. In one case this caused a bug since they used this construct with an integer, not a boolean. But since they assign to booleans and numbers the same way I couldn't tell which type the variable was without digging through the aforementioned struct typedefs - I just did a quick replace. Though I may be TRWTF for not making sure of that before committing the change.

    Bonus WTF: the compiler used for this program is the 2006 version of IAR's C/C++ compiler for ARMs. We're required to turn off all optimization flags during compilation because they "might change the behavior of the program in some way". Since this is the nuclear industry, any amount of uncertainty is completely unacceptable. So unless I want to spend a few weeks digging through assembly to prove that the optimized and non-optimized compiler output are functionally identical I cannot use a new version or enable any optimizations.



  • For programs written by non-computer science folks, that seems fairly innocuous.

    If that's the worst you're seeing, count your blessings.



  • First, it is important to remember that the addition of boolean is relatively recent. There is also a potential for unintended changes in behaviour [which by your own statement is "completely unacceptable"] by making the change from the if construct to the direct assignment. Unless there was an actual defect being caused by the existing implementations, I would have rejected those changes at a code review [based on the information provided]



  • Have to agree with TheCPUWizard. The IF construct is a natural way for an EE to think; ditto they are much more used to thinking of 0 and 1 and not true or false. And in some languages and compilers, true = -1 unless explicitly declared as = 1, which could be literally disastrous if a variable which has 'true' assigned to it is used elsewhere in the program to set bits or states on some kind of interface to hardware.



  • @snoofle said:

    For programs written by non-computer science folks, that seems fairly innocuous.

    If that's the worst you're seeing, count your blessings.

    Oh, I am. Hence the disclaimer.

    @TheCPUWizard said:

    First, it is important to remember that the addition of boolean is relatively recent.

    Yea, but 2006 doesn't exactly predate the boolean type. It's in the compiler manual.

    @TheCPUWizard said:

    There is also a potential for unintended changes in behaviour [which by your own statement is "completely unacceptable"] by making the change from the if construct to the direct assignment.

    There's a difference. What values of GIODINA will cause voltageError, a boolean, to be set to different values between the code examples given above?

    I do see where you're going with that, though.

    @TheCPUWizard said:

    Unless there was an actual defect being caused by the existing implementations, I would have rejected those changes at a code review [based on the information provided]

    There is a need to conserve ROM space where possible. Since I can't turn on compiler optimization, removing BR statements from the generated assembly seems like another good way to save space (to me, anyway).

    @Cad Delworth said:

    And in some languages and compilers, true = -1 unless explicitly declared as = 1, which could be literally disastrous if a variable which has 'true' assigned to it is used elsewhere in the program to set bits or states on some kind of interface to hardware.

    Een-ter-esting, thanks for sharing.



  • @lettucemode said:

    There is a need to conserve ROM space where possible. Since I can't turn on compiler optimization, removing BR statements from the generated assembly seems like another good way to save space (to me, anyway

    Unless you have actually examined the generated code, there is no way to tell if you have actually reduced anything. Remember a boolean datatype is still a byte wide value (they are not packed into individual bits), so any conversion to a boolean must involve some type of conditional, either generated intrnally by the compiler or explicit in the code.

     Being a loong time developer (going back to the mid-1970's) I am very aware of having to make items fit in available memory (want to control a Navy [NATO Walrus class] Submarine with 8K of ROM????) and spend many nights/weekends, getting something down by just a handful of bytes..



  • I was totally underwhelmed by this post until i read
    @lettucemode said:

    Since this is the nuclear industry

    I would have hoped whatever they do there, they have some serious code reviews by competent CS people instead of having EEs whack stuff together. (Not trying to diss the EEs here)



  • @topspin said:

    I was totally underwhelmed by this post until i read
    @lettucemode said:

    Since this is the nuclear industry

    I would have hoped whatever they do there, they have some serious code reviews by competent CS people instead of having EEs whack stuff together. (Not trying to diss the EEs here)

    No worries; this is a non-safety critical system.



  • @lettucemode said:

    I also came across the following construct:

    if (GIODINA & 8 == 8)
        voltageAlarm = 1;
    else
        voltageAlarm = 0;
    WTF, I thought. Never mind that GIODINA is a TI-provided variable of a TI-provided type which allows you to simply say GIODINA.bit3 to get the fourth bit. Why not replace that statement with voltageAlarm = GIODINA & 8;, assigning the bit to the boolean value directly?

    Wouldn't that set voltageAlarm to 8, not 1, if the bit is set?



  • @lettucemode said:

    if (GIODINA & 8 == 8)
    voltageAlarm = 1;
    else
    voltageAlarm = 0;

    The safe optimisation would be

    voltageAlarm = (GIODINA >> 3) & 1;

    But since you say it's from a type which has a bit3 member, why didn't you use that?



  • @lettucemode said:

    Disclaimer: No offense is intended toward the many talented EEs who may or may not be on this forum (fora?). I lived with an EE for three years and he seemed alright. Besides, the things in this post are not that bad, just small errors that I thought were interesting for highlighting the difference between the way the EEs and the rest of us think about programs.

               [ . . . ]

    The next disconnect was sort of amusing: the true and false keywords were nowhere to be found in the program, despite the 100+ boolean variables. Assigning to one of them was done with 1s and 0s, as in voltageError = 1; thingComplete = 0; etc. I assume this was done due to the authors thinking of the booleans directly as bits - which is understandable, given that they're EEs - and ignored the abstraction provided for them. I asked about changing them all to true and false and got a nonenthusiastic response, so I left it.

    An EE is someone who, when you tell him the "true / false / file_not_found" joke, laughs at you, not the joke, for thinking a boolean could have only as few as three states!




  • @DaveK said:

    An EE is someone who, when you tell him the "true / false / file_not_found" joke, laughs at you, not the joke, for thinking a boolean could have only as few as three states!

    As an EE, I feel obligated to prove Dave correct. There is this issue in the field ℤ₂ (although those are not quite Boolean numbers; in ℤ₂, 1+1 = 0) where the equation x² + x + 1 = 0 has no solution. (0×0 + 0 + 1 = 1 = 1×1 + 1 + 1) So we define α as a sort of imaginary value for which α² + α + 1 = 0. Then we need to define another imaginary so that polynomials with 0, 1, & α have solutions, and so ad infinitum.

    Seriously, EEs can make practical use of insanity like this.

    BTW, Dave, say “hi” to my Dad next time you see him.



  • @jcsalomon said:

    There is this issue in the field ℤ₂
     

    Reality is a harsh FILE_NOT_FOUND



  •  @pjt33 said:

    @lettucemode said:

    if (GIODINA & 8 == 8)
    voltageAlarm = 1;
    else
    voltageAlarm = 0;

    The safe optimisation would be

    voltageAlarm = (GIODINA >> 3) & 1;

    But since you say it's from a type which has a bit3 member, why didn't you use that?

     

    How about 

     voltageAlarm = !!(GIODINA & 8)

    for a little fun.

     



  • @UpNDown said:

    How about 

     voltageAlarm = !!(GIODINA & 8)

    for a little fun.

    You seem to thing that's unusual...?



  •  what's wrong with

     

    voltageAlarm = 0 ==(GIODINA & 8);
     
     

     



  • @jes said:

     what's wrong with

     

    voltageAlarm = 0 ==(GIODINA & 8);
     
     

     


    It does the opposite of what it should. That's what's wrong with it.



  •  editing fail on my part:

     

    voltageAlarm = 0 !=(GIODINA & 8);

     does the opposite of my 'does the opposite' suggestion

     



  • Dumb question regarding the redundant Control_setpoints defs, does the target processor have banked memory?

    The redundant fields are probably a WTF, but I could see some gyrations needed if you had to prevent bank switching.

    (disclaimer: Coming from an EE who's made worse WTFs in recent memory).



  • @lettucemode said:

    I also came across the following construct:

    if (GIODINA & 8 == 8)
        voltageAlarm = 1;
    else
        voltageAlarm = 0;
    Bonus WTF: the compiler used for this program is the 2006 version of IAR's C/C++ compiler for ARMs. We're required to turn off all optimization flags during compilation because they "might change the behavior of the program in some way". Since this is the nuclear industry, any amount of uncertainty is completely unacceptable. So unless I want to spend a few weeks digging through assembly to prove that the optimized and non-optimized compiler output are functionally identical I cannot use a new version or enable any optimizations.


    Even a non-optimizing ARM compiler should be able to do that without generating a BR.

    Also, using the bit field access might generate a lot of unexpected code. My experience so far has been that explicit bit masking is never less efficient than using a bit field structure, and can be more efficient (even if less safe and less clear)


  • @DaveK said:

    An EE is someone who, when you tell him the "true / false / file_not_found" joke, laughs at you, not the joke, for thinking a boolean could have only as few as three states!
     

     In VHDL (a language used to describe logic circuits for simulation or implementation) the most common type used to represent a boolean value is std_logic.  It has 9 values:

    • 'U' - Uninitialized
    • 'X' - Unknown
    • '0' - Logic 0
    • '1' - Logic 1
    • 'Z' - High Impedance
    • 'W' - Weak unknown
    • 'H' - Weak Logic 1
    • 'L' - Weal Logic 0
    • '-' - Don't care
    Of course this is trying to map to the real world behaviour of electronic logic ciruits.  For example assigning 'Z' and '1' at the same time is possible and gives '1'.  Assign 'L' and '1' together gives '1'.



  • @ShawnD said:

    @DaveK said:

    An EE is someone who, when you tell him the "true / false / file_not_found" joke, laughs at you, not the joke, for thinking a boolean could have only as few as three states!
     

     In VHDL (a language used to describe logic circuits for simulation or implementation) the most common type used to represent a boolean value is std_logic.  It has 9 values:

    • 'U' - Uninitialized
    • 'X' - Unknown
    • '0' - Logic 0
    • '1' - Logic 1
    • 'Z' - High Impedance
    • 'W' - Weak unknown
    • 'H' - Weak Logic 1
    • 'L' - Weal Logic 0
    • '-' - Don't care
    Of course this is trying to map to the real world behaviour of electronic logic ciruits.  For example assigning 'Z' and '1' at the same time is possible and gives '1'.  Assign 'L' and '1' together gives '1'.
    That was the joke!


  • @Sutherlands said:

    That was the joke!
     

    And he was explaining it, because most of us here aren't EE's. So his post was more useful than yours.

     



  • @Zylon said:

    @Sutherlands said:

    That was the joke!
     

    And he was explaining it, because most of us here aren't EE's. So his post was more useful than yours.

    In TheDailyWTF, most people don't know about VHDL.  They can have a multitude of states, ranging from Don't Care to Care.  Most people fall under Don't Care, but there are still a few that fall in Care.  Those in the Care state have the ability to google the problem to find out more information.  Sometimes, regardless of state, these users will read the tags and get a summary without having to rehash it in a post.


  • Without the explanation, most of us wouldn't have even known to Google "VHDL".

    So I'll put this in terms even you can understand-- Go fuck yourself.

     



  • @Zylon said:

    Without the explanation, most of us wouldn't have even known to Google "VHDL".

    So I'll put this in terms even you can understand-- Go fuck yourself.

     

    Aww, someone's upset.  Here's a hint: Google "electrical engineering multiple boolean states".  Ta-da!  Isn't learning fun?


  • @Sutherlands said:

    Aww, someone's upset.
    Troll identified. No further response required.

     



  • @Zylon said:

    Troll identified. No further response required.

    What?

    This one? 

    @Sutherlands said:

    Aww, someone's upset.

    Or this?

    @Zylon said:

    Go fuck yourself

     



  • @C-Octothorpe said:

    @Zylon said:

    Troll identified. No further response required.

    What?

    This one? 

    @Sutherlands said:

    Aww, someone's upset.

    Or this?

    @Zylon said:

    Go fuck yourself

    +1


  • @Sutherlands said:

    In TheDailyWTF, most people don't know about VHDL.  They can have a multitude of states, ranging from Don't Care to Care.  Most people fall under Don't Care, but there are still a few that fall in Care.  Those in the Care state have the ability to google the problem to find out more information.  Sometimes, regardless of state, these users will read the tags and get a summary without having to rehash it in a post.

    I certainly didn't care enough to Google it, but I did find ShawnD's post an informative curio.



  • @morbiuswilters said:

    @Sutherlands said:
    In TheDailyWTF, most people don't know about VHDL.  They can have a multitude of states, ranging from Don't Care to Care.  Most people fall under Don't Care, but there are still a few that fall in Care.  Those in the Care state have the ability to google the problem to find out more information.  Sometimes, regardless of state, these users will read the tags and get a summary without having to rehash it in a post.
    I certainly didn't care enough to Google it, but I did find ShawnD's post an informative curio.
    The only reason I posted that is because that is because the same information has been posted in the boards multiple times somewhat recently.  From what I gather, you haven't been here, so of course you wouldn't see them, but I feel like most other regulars should have seen at least one post. 


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.