CppCheck cleanup (#975)

* Add support for inline cppcheck suppressions
* Clean out all known cppcheck issues
* Terminate toll-gate on found cppcheck issues

.ci/static_analysis.groovy

@@ -5,7 +5,7 @@ def cppCheck(config) {
 	sh """#!/bin/bash +x
 				cd ${config.repository_root}
 				echo "Doing cppcheck for AVR..."
-				find . -type f \\( -iname \\*.c -o -iname \\*.cpp -o -iname \\*.ino \\) | cppcheck -j 4 --file-list=- --enable=style,information --platform=.mystools/cppcheck/config/avr.xml --suppressions-list=.mystools/cppcheck/config/suppressions.cfg --includes-file=.mystools/cppcheck/config/includes.cfg --language=c++ --xml --xml-version=2 2> cppcheck-avr.xml
+				find . -type f \\( -iname \\*.c -o -iname \\*.cpp -o -iname \\*.ino \\) | cppcheck -j 4 --force --file-list=- --enable=style,information --platform=.mystools/cppcheck/config/avr.xml --suppressions-list=.mystools/cppcheck/config/suppressions.cfg --includes-file=.mystools/cppcheck/config/includes.cfg --language=c++ --inline-suppr --xml --xml-version=2 2> cppcheck-avr.xml
 				cppcheck-htmlreport --file="cppcheck-avr.xml" --title="cppcheck-avr" --report-dir=cppcheck-avr_cppcheck_reports --source-dir=."""
 
 	publishHTML([allowMissing: false, alwaysLinkToLastBuild: false, keepAll: true,
@@ -29,9 +29,8 @@ def cppCheck(config) {
 				"grep -q \"<td>0</td><td>total</td>\" cppcheck-avr_cppcheck_reports/index.html || exit_code=\$?\n"+
 				"exit \$((exit_code == 0 ? 0 : 1))")
 	if (ret == 1) {
-		config.pr.setBuildStatus(config, 'SUCCESS', 'Toll gate (Code analysis - Cppcheck)', 'Issues found (but are not considered critical)', '${BUILD_URL}CppCheck_AVR/index.html')
-		//currentBuild.result = 'UNSTABLE'
-		//error 'Terminating due to Cppcheck error'
+		config.pr.setBuildStatus(config, 'ERROR', 'Toll gate (Code analysis - Cppcheck)', 'Issues found', '${BUILD_URL}CppCheck_AVR/index.html')
+		error 'Terminating due to Cppcheck error'
 	} else {
 		config.pr.setBuildStatus(config, 'SUCCESS', 'Toll gate (Code analysis - Cppcheck)', 'Pass', '')
 	}

.clang_complete

@@ -1,8 +1,3 @@
--IC:\Program Files (x86)\Arduino\hardware\arduino\avr\cores\arduino
--IC:\Program Files (x86)\Arduino\hardware\tools\avr\avr\include
 -Idrivers\Linux
 -Idrivers\ATSHA204
 -Icore
--DMY_SIGNING_ATSHA204
--DMY_SIGNING_SOFT
--DARDUINO_ARCH_AVR

.mystools/cppcheck/config/suppressions.cfg

@@ -1,5 +1,4 @@
-toomanyconfigs
-missingInclude
-missingIncludeSystem
 ConfigurationNotChecked
 unmatchedSuppression
+// This suppression is because the problem is in an in-lined macro so in-line-suppressions does not appear to take effect
+unreadVariable:*/MyHwNRF5.cpp

.mystools/cppcheck/options.sh

@@ -9,6 +9,7 @@ OPTIONS="--quiet                                      \
 	--library=${LIBRARY:-avr}                           \
 	--platform="${TOOLCONFIG}"/${PLATFORM:-avr.xml}     \
  	--includes-file="${TOOLCONFIG}"/includes.cfg	      \
+ 	--inline-suppr                                      \
 	--suppressions-list="${TOOLCONFIG}"/suppressions.cfg"
 
 echo $OPTIONS

core/MyGatewayTransportEthernet.cpp

@@ -49,7 +49,11 @@ MyMessage _ethernetMsg;
 #define ARRAY_SIZE(x)  (sizeof(x)/sizeof(x[0]))
 
 typedef struct {
+	// Suppress the warning about unused members in this struct because it is used through a complex
+	// set of preprocessor directives
+	// cppcheck-suppress unusedStructMember
 	char string[MY_GATEWAY_MAX_RECEIVE_LENGTH];
+	// cppcheck-suppress unusedStructMember
 	uint8_t idx;
 } inputBuffer;
 

core/MyLeds.cpp

@@ -58,7 +58,9 @@ void ledsProcess()
 	}
 	prevTime = hwMillis();
 
+#if defined(MY_DEFAULT_RX_LED_PIN) || defined(MY_DEFAULT_TX_LED_PIN) || defined(MY_DEFAULT_ERR_LED_PIN)
 	uint8_t state;
+#endif
 
 	// For an On/Off ratio of 4, the pattern repeated will be [on, on, on, off]
 	// until the counter becomes 0.

core/MyOTAFirmwareUpdate.cpp

@@ -128,7 +128,6 @@ bool firmwareOTAUpdateProcess(void)
 					MY_SERIALDEVICE.print(prbuf);
 				}
 				OTA_DEBUG(PSTR("\n"));
-				8
 			}
 #endif
 			_firmwareBlock--;

core/MyProtocolMySensors.cpp

@@ -59,7 +59,6 @@ bool protocolParse(MyMessage &message, char *inputString)
 			break;
 		case 5: // Variable value
 			if (command == C_STREAM) {
-				blen = 0;
 				while (*str) {
 					uint8_t val;
 					val = protocolH2i(*str++) << 4;
@@ -125,8 +124,6 @@ bool protocolMQTTParse(MyMessage &message, char* topic, uint8_t* payload, unsign
 {
 	char *str, *p;
 	uint8_t i = 0;
-	uint8_t bvalue[MAX_PAYLOAD];
-	uint8_t blen = 0;
 	uint8_t command = 0;
 	if (topic != strstr(topic, MY_MQTT_SUBSCRIBE_TOPIC_PREFIX)) {
 		// Prefix doesn't match incoming topic
@@ -175,9 +172,10 @@ bool protocolMQTTParse(MyMessage &message, char* topic, uint8_t* payload, unsign
 
 	// Add payload
 	if (command == C_STREAM) {
-		blen = 0;
-		uint8_t val;
+		uint8_t bvalue[MAX_PAYLOAD];
+		uint8_t blen = 0;
 		while (*payload) {
+			uint8_t val;
 			val = protocolH2i(*payload++) << 4;
 			val += protocolH2i(*payload++);
 			bvalue[blen] = val;

core/MySigning.cpp

@@ -111,6 +111,8 @@ void signerInit(void)
 #endif
 #if (defined (MY_ENCRYPTION_FEATURE) || defined (MY_SIGNING_FEATURE)) &&\
     !defined (MY_SIGNING_SIMPLE_PASSWD)
+	// Suppress this warning since it is only fixed on Linux builds and this keeps the code more tidy
+	// cppcheck-suppress knownConditionTrueFalse
 	if (!signerInternalValidatePersonalization()) {
 		SIGN_DEBUG(PSTR("!SGN:PER:TAMPERED\n"));
 #if defined(MY_SIGNING_FEATURE)

core/MySigningAtsha204Soft.cpp

@@ -103,10 +103,8 @@ bool signerAtsha204SoftInit(void)
 		_signing_node_serial_info[8] = getNodeId();
 	}
 #else
-	if (init_ok) {
-		hwReadConfigBlock((void*)_signing_hmac_key, (void*)EEPROM_SIGNING_SOFT_HMAC_KEY_ADDRESS, 32);
-		hwReadConfigBlock((void*)_signing_node_serial_info, (void*)EEPROM_SIGNING_SOFT_SERIAL_ADDRESS, 9);
-	}
+	hwReadConfigBlock((void*)_signing_hmac_key, (void*)EEPROM_SIGNING_SOFT_HMAC_KEY_ADDRESS, 32);
+	hwReadConfigBlock((void*)_signing_node_serial_info, (void*)EEPROM_SIGNING_SOFT_SERIAL_ADDRESS, 9);
 #endif
 	if (!memcmp(_signing_node_serial_info, reset_serial, 9)) {
 		unique_id_t uniqueID;

core/MyTransport.cpp

@@ -940,6 +940,8 @@ void transportProcessMessage(void)
 
 void transportInvokeSanityCheck(void)
 {
+	// Suppress this because the function may return a variable value in some configurations
+	// cppcheck-suppress knownConditionTrueFalse
 	if (!transportSanityCheck()) {
 		TRANSPORT_DEBUG(PSTR("!TSF:SAN:FAIL\n"));	// sanity check fail
 		transportSwitchSM(stFailure);

drivers/AES/AES.cpp

@@ -407,19 +407,19 @@ byte AES::cbc_encrypt (byte * plain, byte * cipher, int n_block)
 
 /******************************************************************************/
 
-byte AES::decrypt (byte plain [N_BLOCK], byte cipher [N_BLOCK])
+byte AES::decrypt (byte cipher [N_BLOCK], byte plain [N_BLOCK])
 {
 	if (round) {
 		byte s1 [N_BLOCK] ;
-		copy_and_key (s1, plain, (byte*) (key_sched + round * N_BLOCK)) ;
+		copy_and_key (s1, cipher, (byte*) (key_sched + round * N_BLOCK)) ;
 		inv_shift_sub_rows (s1) ;
 
 		for (byte r = round ; --r ; ) {
 			byte s2 [N_BLOCK] ;
 			copy_and_key (s2, s1, (byte*) (key_sched + r * N_BLOCK)) ;
 			inv_mix_sub_columns (s1, s2) ;
 		}
-		copy_and_key (cipher, s1, (byte*) (key_sched)) ;
+		copy_and_key (plain, s1, (byte*) (key_sched)) ;
 	} else {
 		return AES_FAILURE ;
 	}

drivers/ATSHA204/sha256.cpp

@@ -30,6 +30,10 @@ const uint8_t sha256InitState[] PROGMEM = {
 	0x19,0xcd,0xe0,0x5b  // H7
 };
 
+// Suppress warning about uninitialized variables because initializing them in an init function
+// allows the compiler to optimize away the variables in case the class is only instantiated but
+// never used.
+// cppcheck-suppress uninitMemberVar
 Sha256Class::Sha256Class()
 {
 	/*
@@ -165,6 +169,10 @@ uint8_t* Sha256Class::result(void)
 #define HMAC_IPAD 0x36
 #define HMAC_OPAD 0x5c
 
+// Suppress warning about uninitialized variables because initializing them in an init function
+// allows the compiler to optimize away the variables in case the class is only instantiated but
+// never used.
+// cppcheck-suppress uninitMemberVar
 HmacClass::HmacClass()
 {
 }

drivers/AVR/DigitalIO/PinIO.h

@@ -41,6 +41,10 @@ class PinIO
 {
 public:
 	/** Create a PinIO object with no assigned pin. */
+	// Suppress warning about uninitialized variables because initializing them in an init function
+	// allows the compiler to optimize away the variables in case the class is only instantiated but
+	// never used.
+	// cppcheck-suppress uninitMemberVar
 	PinIO() : bit_(0), mask_(0XFF) {}
 	/** Constructor
 	 * @param[in] pin Pin assigned to this object.
@@ -186,4 +190,4 @@ class PinIO
 	volatile uint8_t* portReg_;
 };
 #endif  // PinIO_h
-/** @} */
+/** @} */

drivers/AVR/DigitalIO/examples/SoftDS1307Utility/SoftDS1307Utility.ino

@@ -179,7 +179,6 @@ void dumpAll(void)
 //------------------------------------------------------------------------------
 void fillNvRam(void)
 {
-	uint8_t buf[8];
 	PgmPrint("Enter HEX value for all NV RAM locations (00-FF): ");
 	uint16_t v;
 	if (!hexRead(&v)) {
@@ -363,4 +362,4 @@ void loop(void)
 	} else {
 		PgmPrintln("Invalid option");
 	}
-}
+}

drivers/AltSoftSerial/AltSoftSerial.cpp

@@ -131,13 +131,14 @@ void AltSoftSerial::writeByte(uint8_t b)
 
 ISR(COMPARE_A_INTERRUPT)
 {
-	uint8_t state, byte, bit, head, tail;
+	uint8_t state, byte, head, tail;
 	uint16_t target;
 
 	state = tx_state;
 	byte = tx_byte;
 	target = GET_COMPARE_A();
 	while (state < 9) {
+		uint8_t bit;
 		target += ticks_per_bit;
 		bit = byte & 1;
 		byte >>= 1;
@@ -195,9 +196,8 @@ void AltSoftSerial::flushOutput(void)
 
 ISR(CAPTURE_INTERRUPT)
 {
-	uint8_t state, bit, head;
-	uint16_t capture, target;
-	int16_t offset;
+	uint8_t state, bit;
+	uint16_t capture;
 
 	capture = GET_INPUT_CAPTURE();
 	bit = rx_bit;
@@ -217,8 +217,10 @@ ISR(CAPTURE_INTERRUPT)
 			rx_state = 1;
 		}
 	} else {
+		uint16_t target;
 		target = rx_target;
 		while (1) {
+			int16_t offset;
 			offset = capture - target;
 			if (offset < 0) {
 				break;
@@ -227,6 +229,7 @@ ISR(CAPTURE_INTERRUPT)
 			target += ticks_per_bit;
 			state++;
 			if (state >= 9) {
+				uint8_t head;
 				DISABLE_INT_COMPARE_B();
 				head = rx_buffer_head + 1;
 				if (head >= RX_BUFFER_SIZE) {

drivers/BCM/bcm2835.c

@@ -113,11 +113,11 @@ unsigned int bcm2835_version(void)
  */
 uint32_t bcm2835_peri_read(volatile uint32_t* paddr)
 {
-	uint32_t ret;
 	if (debug) {
 		printf("bcm2835_peri_read  paddr %08X\n", (unsigned) paddr);
 		return 0;
 	} else {
+		uint32_t ret;
 		__sync_synchronize();
 		ret = *paddr;
 		__sync_synchronize();
@@ -438,7 +438,7 @@ void bcm2835_delayMicroseconds(uint64_t micros)
 
 	if (debug) {
 		/* Cant access sytem timers in debug mode */
-		printf("bcm2835_delayMicroseconds %lld\n", micros);
+		printf("bcm2835_delayMicroseconds %" PRIu64 "\n", micros);
 		return;
 	}
 

drivers/Linux/Stream.cpp

@@ -30,9 +30,9 @@
 // private method to read stream with timeout
 int Stream::timedRead()
 {
-	int c;
 	_startMillis = millis();
 	do {
+		int c;
 		c = read();
 		if(c >= 0) {
 			return c;
@@ -45,9 +45,9 @@ int Stream::timedRead()
 // private method to peek stream with timeout
 int Stream::timedPeek()
 {
-	int c;
 	_startMillis = millis();
 	do {
+		int c;
 		c = peek();
 		if(c >= 0) {
 			return c;
@@ -61,8 +61,8 @@ int Stream::timedPeek()
 // discards non-numeric characters
 int Stream::peekNextDigit()
 {
-	int c;
 	while(1) {
+		int c;
 		c = timedPeek();
 		if(c < 0) {
 			return c;  // timeout