Milestone: getting to version 0.1.0 about sensor-watch HOT 9 CLOSED

Very cool! I need to chew on some of these, but for the moment I'm having some challenges getting this integrated into the Makefile. What does the metrics argument mean in there? The output I get right now is:

$ make analyze
cobra: cannot find 'metrics'

from sensor-watch.

Thanks so much, this is very helpful! It did expose several actionable things, both style and substance. One of the inconsistently checked return value warnings helped explain a glitch in the World Clock when exiting settings mode! And it also seemed wise to add comments to those empty loops to make it clear what they're for.

One other thing it exposed is some lazy coding on my part: the underlying IO calls for SERCOMs (SPI, I2C, UART) return status codes to indicate whether reads and writes were successful. Ideally, the watch API for this would return at least boolean values for success or failure, but as of now all my calls are void (or in the case of watch_uart_getc, the received character). This feels like a good place for future improvement, but it's not something I addressed in this round.

from sensor-watch.

Hi Joey, as far as suggestions would you want to perform any static analysis before this release?

from sensor-watch.

I would love to perform static analysis; I'm down with anything that improves the firmware's reliability and correctness. Do you have a particular tool that you would suggest using?

from sensor-watch.

I really like Cobra.

I use the built in rule sets, not the interactive portion (though that is cool too).

I'm not sure if you want to try to adhere to MISRA or anything, but the basic ruleset and cwe are pretty good. I can help configure it if you want to try it out!

I am not sure how this is usually done with Make, but I added a rule

COBRA = cobra -f
...
analyze:
	@$(COBRA) metrics $(INCLUDES) $(DEFINES) $(SRCS)

and tested with it, but I am getting an error from Cobra that is going to be fixed with the next release.

from sensor-watch.

I got this working in the previous version of Cobra. Here's an attachment of the basic ruleset output.

cobra_basic.txt

You can find each rule's set of results by searching for the "===" indicator.

I'm not sure what is happening with the "bad field type 'fct'" lines.

Adding MISRA 2012 for fun.

cobra_misra2012.txt

from sensor-watch.

'metrics' is the rule file that Cobra is wanting to use. There are a few rulesets that come with Cobra. I ran 'basic' and 'misra2012'. Typically I run all of them but I didn't add all of that to the Makefile yet.

There is a configuration step in the Cobra docs that puts the path to the rulesets where Cobra can see them. Did you run that step?

It is on this page -- cobra -configure $COBRA/rules

from sensor-watch.

Ah, got it! I had put the binaries in kind of an odd spot so the rules directory was configured awkwardly. Also it looks like the bad field type 'fct' lines aren't going to stdout (maybe to stderr?) so they're likely not related to the analysis.

There are definitely some actionable things in there, but I'm a bit confused by some of the other results; for example, I'm getting several of these:

=== Inconsistent checks of function return values:
 1: _update checked 2 times out of 4

The thing is, _update is a void function and we never seem to check the return value at all. I can silence the warning by changing lines like

if ((date_time.unit.minute == 0) && (date_time.unit.second == 0)) _update(settings, state, state->offset);

to

if ((date_time.unit.minute == 0) && (date_time.unit.second == 0)) {
    _update(settings, state, state->offset);
}

but I'm unclear as to whether what I was doing previously is actually incorrect, or if this is a false positive.

Similarly it's firing on some "empty while statements" like while(!SUPC->STATUS.bit.VREGRDY);. I get this one: since the condition is just a bit in memory, static analysis sees it as a while(0) or while(1). But since the register's value will change when the underlying state of the hardware changes, this is actually correct.

Basically I'm wondering how I should interpret what it's telling me, whether the goal is to get to zero warnings, and if in cases where some of what's there is known to be correct, if there's a way to tell this to the tool so it can become part of the build steps and alert us when something is really off.

from sensor-watch.

Hey there. Sorry for the delayed reply.

Great point about the lines potentially going to stderr.

Yes, so I have used Cobra on a few projects, and it does generate some false positives, The void function one appears to be one.

For the while loops, it isn't saying that the value won't change, just that the while doesn't contain any statements, like there are no curly braces that contain code to be executed as part of the while. This could be an error, but in your case it is correct. I get it for the same reason though -- waiting on a register to change.

The goal is not to get zero warnings, and yes there are cases where it can be incorrect. I always check the results however to determine for myself if it is a false positive. So far there isn't a way that I know of to silence any particular warning though there is an effort to have Cobra support SARIF which would allow it to work with some static analysis tool sets.

Basically I like using Cobra since it is freely available and has caught many true issues when using CWE/MISRA/JPL/P10 rulesets. Though you have described some of the rough edges. I have a tool (that I unfortunately can't open source at the moment) that interprets Cobra logs and allows you do perform some filtering on them to make things a little easier in the meantime.

from sensor-watch.