Fix a few PR suggestions:

- Prefix types with `std::`. - Comment `| 0`. - Still using explicit width types, because they're what the API takes (and none are exposed outside the plugin). - Not a suggestion, but I switched to using `<< 11` and `<< 29` instead of more magic numbers to make it slightly clearer that those numbers are to do with CAN 11- and 29-bit identifiers. - Reduced some scopes.
Open-Agriculture · Mar 9, 2023 · 3472763 · 3472763
1 parent 9d95a06
commit 3472763
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 25 deletions.
diff --git a/hardware_integration/include/isobus/hardware_integration/ntcan_fifo_plugin.hpp b/hardware_integration/include/isobus/hardware_integration/ntcan_fifo_plugin.hpp
@@ -31,7 +31,7 @@ class NTCANFIFOPlugin : public CANHardwarePlugin
 	explicit NTCANFIFOPlugin(int channel);
 
 	/// @brief The destructor for NTCANFIFOPlugin
-	virtual ~NTCANFIFOPlugin();
+	virtual ~NTCANFIFOPlugin() = default;
 
 	/// @brief Returns if the connection with the hardware is valid
 	/// @returns `true` if connected, `false` if not connected
@@ -55,8 +55,8 @@ class NTCANFIFOPlugin : public CANHardwarePlugin
 
 private:
 	int net;
-	uint64_t timestampFreq;
-	uint64_t timestampOff;
+	std::uint64_t timestampFreq;
+	std::uint64_t timestampOff;
 	NTCAN_HANDLE handle; ///< The handle as defined in the NTCAN FIFO driver API
 	NTCAN_RESULT openResult; ///< Stores the result of the call to begin CAN communication. Used for is_valid check later.
 };

diff --git a/hardware_integration/src/ntcan_fifo_plugin.cpp b/hardware_integration/src/ntcan_fifo_plugin.cpp
@@ -24,10 +24,6 @@ NTCANFIFOPlugin::NTCANFIFOPlugin(int channel) :
 {
 }
 
-NTCANFIFOPlugin::~NTCANFIFOPlugin()
-{
-}
-
 bool NTCANFIFOPlugin::get_is_valid() const
 {
 	return (NTCAN_SUCCESS == openResult);
@@ -40,19 +36,20 @@ void NTCANFIFOPlugin::close()
 
 void NTCANFIFOPlugin::open()
 {
-	uint32_t mode = 0;
-	int32_t txQueueSize = 8;
-	int32_t rxQueueSize = 8;
-	int32_t txTimeOut = 100;
-	int32_t rxTimeOut = 1000;
-	CAN_IF_STATUS status {0};
-	uint64_t timestamp = 0;
-	int ids;
+	{
+		std::uint32_t mode = 0;
+		std::int32_t txQueueSize = 8;
+		std::int32_t rxQueueSize = 8;
+		std::int32_t txTimeOut = 100;
+		std::int32_t rxTimeOut = 1000;
 
-	openResult = canOpen(net, mode, txQueueSize, rxQueueSize, txTimeOut, rxTimeOut, &handle);
+		openResult = canOpen(net, mode, txQueueSize, rxQueueSize, txTimeOut, rxTimeOut, &handle);
+	}
 
 	if (NTCAN_SUCCESS == openResult)
 	{
+		CAN_IF_STATUS status {0};
+
 		openResult = canSetBaudrate(handle, NTCAN_BAUD_250);
 
 		if (NTCAN_SUCCESS == openResult)
@@ -62,6 +59,7 @@ void NTCANFIFOPlugin::open()
 
 		if (NTCAN_FEATURE_TIMESTAMP == (status.features & NTCAN_FEATURE_TIMESTAMP))
 		{
+			std::uint64_t timestamp = 0;
 			if (NTCAN_SUCCESS == openResult)
 			{
 				openResult = canIoctl(handle, NTCAN_IOCTL_GET_TIMESTAMP_FREQ, &timestampFreq);
@@ -76,26 +74,27 @@ void NTCANFIFOPlugin::open()
 			{
 				auto now = std::chrono::system_clock::now();
 				auto unix = now.time_since_epoch();
-				uint64_t millis = std::chrono::duration_cast<std::chrono::microseconds>(unix).count();
+				long long millis = std::chrono::duration_cast<std::chrono::microseconds>(unix).count();
 				timestampOff = millis - timestamp;
 			}
 		}
 
 		if (NTCAN_SUCCESS == openResult)
 		{
-			ids = 0x800;
+			std::int32_t ids = (1 << 11);
 			openResult = canIdRegionAdd(handle, 0, &ids);
-			if (NTCAN_SUCCESS == openResult && ids != 0x800)
+			if (NTCAN_SUCCESS == openResult && ids != (1 << 11))
 			{
 				openResult = NTCAN_INSUFFICIENT_RESOURCES;
 			}
 		}
 
 		if (NTCAN_SUCCESS == openResult)
 		{
-			ids = 0x20000000;
+			std::int32_t ids = (1 << 29);
+			// Address 0, with the wide address flag.
 			openResult = canIdRegionAdd(handle, 0 | NTCAN_20B_BASE, &ids);
-			if (NTCAN_SUCCESS == openResult && ids != 0x20000000)
+			if (NTCAN_SUCCESS == openResult && ids != (1 << 29))
 			{
 				openResult = NTCAN_INSUFFICIENT_RESOURCES;
 			}
@@ -118,15 +117,15 @@ bool NTCANFIFOPlugin::read_frame(isobus::HardwareInterfaceCANFrame &canFrame)
 	NTCAN_RESULT result;
 	CMSG_T msgCanMessage {0};
 	bool retVal = false;
-	int32_t count = 1;
+	std::int32_t count = 1;
 
 	result = canReadT(handle, &msgCanMessage, &count, nullptr);
 
 	if (NTCAN_SUCCESS == result && 1 == count)
 	{
 		canFrame.dataLength = msgCanMessage.len;
 		memcpy(canFrame.data, msgCanMessage.data, msgCanMessage.len);
-		canFrame.identifier = (msgCanMessage.id & 0x1FFFFFFF);
+		canFrame.identifier = (msgCanMessage.id & ((1 << 29) - 1));
 		canFrame.isExtendedFrame = (NTCAN_20B_BASE == (msgCanMessage.id & NTCAN_20B_BASE));
 		canFrame.timestamp_us = msgCanMessage.timestamp * 1000000 / timestampFreq + timestampOff;
 		retVal = true;
@@ -142,9 +141,9 @@ bool NTCANFIFOPlugin::write_frame(const isobus::HardwareInterfaceCANFrame &canFr
 {
 	NTCAN_RESULT result;
 	CMSG_T msgCanMessage {0};
-	int32_t count = 1;
+	std::int32_t count = 1;
 
-	msgCanMessage.id = canFrame.isExtendedFrame ? canFrame.identifier | NTCAN_20B_BASE : canFrame.identifier;
+	msgCanMessage.id = canFrame.isExtendedFrame ? (canFrame.identifier | NTCAN_20B_BASE) : canFrame.identifier;
 	msgCanMessage.len = canFrame.dataLength;
 	memcpy(msgCanMessage.data, canFrame.data, canFrame.dataLength);