TivaWare bug

Similarly in tm4c1230e6pm.h and I suspect all of the processor headers.

 

Robert

Robert Adsett said:

While this is correct in the sense that it will do the job is does violate MISRA.  MISRA requires a U suffix to indicate that the constant is unsigned.  

It is easily fixed but for obvious reasons it does not make sense for me to fix it here.

I am not sure if TI claim that any of their tools / libraries are MISRA compliant. E.g. the We wish to comply with an Industry Standard XX to justify safety or industry specific requirements for the tools we use. Is there a validation report or certificate available for the compiler? (XX= IEC61508; TÜV; MISRA-C, etc) section of the Code Generation Tools FAQ states:

Special conformance testing of tools, compilers and runtime support libraries is the responsibility of the user.

Therefore, not sure how likely it is that TI will change the header (and source files) in TivaWare to be MISRA compliant.

Oh, I'm not expecting it to be fixed. I'm expecting to be completely ignored.  I just thought it was worth reporting since TivaWare so far appears to be  a reasonable starting point unlike most vendor code I have worked with.

As far as Misra compliance goes I only used it as a short cut since their reasoning for the rules is readily available and it meant I didn't need to explain why I considered it a bug. So let me do that.

The processor peripheral registers are declared as unsigned integers (uint32_t or somesuch IIRC).  This reasonable and logical since there is no obvious way to interpret signedness for them.

I take it as given that you should assign the same type to a peripheral as was used to define it. (In fact this can be used to do additional checking with judicious use of typedefs but this does take quite a bit of effort to set up)

Essentially what the TivaWare libraries do is define signed constants that are bit wise orred (problematic for other considerations) and then assigned to an unsigned register relying on implicit conversions.

There is no reason for these constants to be signed and logically signedness make no sense for them.  They are logically unsigned values.

Given both of those considerations the constants should be defined as unsigned.

Now, why would I care about this since it will obviously work? As I said previously I use lint heavily and I rely on it to help me produce quality code. One of the ways it does this is by helping ensure consistent use of types (something it has gotten better at over the decades). Many of the times if you find yourself with a signed/unsigned conversion it is a symptom of an underlying inconsistency and easy to address when found before compiling but very difficult to detect and fix if left to execution time.  Worse, if there is a lot of inconsistency or types shift back and forth it is a near certain sign of a design/algorithm issue.

If these inconsistencies are hidden by a legion of inconsistencies from an insufficiently careful library implementation these benefits are lost.

That is why I consider this an important issue and worth bringing up giving how relatively straightforward I expect it should be to fix.

I have hidden other issues from analysis in such a way that I still get the benefits in the application of the analysis and will continue to search for a way do so for this issue. It really should be made completely internally and explicitly consistent in any case but truthfully I'm not expecting that to happen.

Robert

BTW if anyone wants a copy of what I end up with to silence lint on TivaWare issues let me know and I'll try to post it here. It probably won't be fully complete but it might be a starting point.

Most excellent tech. analysis and over-view/commentary - much appreciated - thank you.

Having engineering & law background - your, "inconsistencies are hidden by a legion of inconsistencies - from an insufficiently careful library implementation" surely rings this sw juror's bell.  That precise choice of language was very well crafted - drove home your point with force...

And - "easy to address when found before compiling - but very difficult to detect & fix (if escaped)" reveals your battle history & desire to emplace safeguards.  Bravo...

Should note that any vendor's, "statement and/or suggestion of compliance" with formal rule/regulation too often opens them to unwanted legal exposure...  And do recall - SW in question is provided w/out charge...

Reasonably sure that many here would benefit from your, "silent lint" effort - look forward to its arrival - again thanks!

Thanks,

 

I've just been working with pin toggling demos up until this morning.

 

I just started trying something a little more complex and made my first call to a TivaWare function with header defined constants.  Of course the function is prototyped as taking an unsigned argument and the constants are signed.

 

Robert

Hi Robert,

I'm relatively to the TivaWare libraries, and they've been a good place to start coding, but to continue using it in production work, I'd need to have higher reliability and stability.  I've already found (&posted about) a bug in the sensorlib; it was a rather simple mistake that could have been caught with basic testing, so the code quality definitely concerns me.

I am curious about other "known" bugs, since there doesn't seem to be any public/member-visible bug tracking.  I'm maintaining my own git repository to fix/track bugs.

I'd like to see the "clean-up" work you've done to the library and try to incorporate it into my repository.  Could you send it to me?

Regards,
Doug

I did promise to post my results and it's now stabilzed enough to do that.  It does only cover what I've run across rather than attempting to correct the entire library which would take more effort than is reasonable for a single project.

I'm actually quite happy with the TIVAWare library but it could use a pass to correct a few items, particularly with regard to types.

There multiple ways bug fixes to the library could be done

• Shell functions and headers that wrap the offending pieces in corrective code
• Replacement pieces
• Correcting the library

I would hesitate to correct the library itself since that will cause a big maintenance issue as libraries are updated.  So that leaves the first two.

The approach I've taken is to resolve the lint issues and is basically a variant on the first option.  PC-Lint allows a good deal of custom control over the complaints it issues (one purpose is precisely for 'broken' libraries like this) so I've made use of that facility.

So given those caveats see the items below set up as a PC-lint "indirect lnt file"  (I'll try to add commentary and remove project specific items).  I have not covered all the issues simply because I have not run across them all.    It's also possible some of these are wrong.

 

Robert

     
 // Not much way around this one unless you use linker scripts.  In some ways that is my prefered method
--elibmacro((923)) // int to pointer cast

 // No I never really expected MISRC compliance for libraries.
-elib(960)  // No MISRA compliance expected
--elibmacro((960)) // No MISRA compliance expected
--elibmacro({960}) // No MISRA compliance expected

 // Signed items that should unsigned
 // Yes this will produce the correct result MOST of the time but not
 // necessarily all of the time.  Besides it fails the "clarity of intent"
 // rule
--emacro((917),*_BASE) // Base offset macros declared as signed, should be unsigned.
--emacro((835),ADC_CTL_*)// Some CTL macros are zero (and then orred to produce control).
--emacro((912),CAN_INT_*)// CAN_INT macros declared as signed, should be unsigned.
--emacro((912),GPIO_*) // GPIO macros declared as signed, should be unsigned.
--emacro((915),GPIO_*) // GPIO macros declared as signed, should be unsigned.
--emacro((917),GPIO_*) // GPIO macros declared as signed, should be unsigned.
--emacro((917),INT_*) // INT macros declared as signed, should be unsigned.
--emacro((912),MSG_OBJ_*)// MSG_OBJ macros declared as signed, should be unsigned.
--emacro((915),MSG_OBJ_*)// MSG_OBJ macros declared as signed, should be unsigned.
--emacro((912),NVIC_*) // NVIC macros declared as signed, should be unsigned.
--emacro((835),SYSCTL_*)// Raised when 0 values macros are orred.
--emacro((912),SYSCTL_*)// SYSCTL macros declared as signed, should be unsigned.
--emacro((915),SYSCTL_*)// SYSCTL macros declared as signed, should be unsigned.
--emacro((917),SYSCTL_*)// SYSCTL macros declared as signed, should be unsigned.
--emacro((9048),SYSCTL_*)// SYSCTL macros have unsigned literal w/o U suffix.
--emacro((9048),NVIC_CPAC)// SYSCTL macros have unsigned literal w/o U suffix.
-elib(9048) // Unsigned literal w/o U suffix
-elib(9022) // Unparenthezised macro parameter

 // Non-ANSI or extension dependent items
-esym(960,5.1)  // TIVA uses long names
-elib(950)  // TIVA non-ANSI constructs notably single line comments
-elib(659)  // Nothing follows } on line terminating struct/union/enum definition.
 // These  non-ansi items particular ones can be hard to meet
-"esym(793,significant characters in an external identifier)" //ANSI identifier length limit of 6 for external identifiers 

 // Bad style, prone to produce bugs
-elib(9050) // Dependence on operator precedence
-elib(9057) // Lowercase L follws u in suffix
-elib(9059) // C comment contains c++ comment (It's possible this one is not TIVA but another library)

-"esym(793,macros in module)"  // ANSI/ISO limit of 1024 macros in a module exceeded by TIVA headers

// Finally, worth avoiding if possible but sometimes it is difficult

-e966    // indirectly included header not used 

Thanks Robert!

That's great.  As for which way to fix bugs in the library, I'm using "git" to track my changes, so I can create diffs to examine later changes.  It also allows me to add/overlay new library versions, then merge, accept or reject the changes.  Any conflicts (like code I've changed that never got back to the TI engineers) can be highlighted, for me to accept (if they've fixed a problem more elegantly), or reject (if they haven't actually fixed the problem).

So we've chosen different methods, but I can see how your changes are far more invasive ("U" on the end of most constants, for example) and would be much more difficult to "merge".

Another group of users is working on resolving (probably computational, maybe chip config/prog) errors in the Sensor library and I'd really like to see their work incorporated back into the library.  If TI starts to take the external bug fixing seriously, then maybe they will set up a repository that we can contribute to.  I can't see this library being open source'd (because of the ITAR limitations), but it's development/improvement would be much faster that way.

Cheers,
Doug