NTCAN hardware layer #198

Y-Less · 2023-03-03T02:11:17Z

I wanted to add support for this virtual CAN bus:

I know there's a basic virtual CAN already in the library, but it doesn't work between processes.

This is only a very basic first pass at the code (and clearly copied almost wholesale from the PCAN implementation), just here for any early comments (I've not even tested it yet). They do claim that their library is cross-platform, so there may be a way to support Linux in here as well. You may notice that the bundled .lib files are in sub-directories instead of using the existing _x86 and _x64 naming convention on other libraries. This is because the include itself, copied from their SDK, looks for a file named exactly ntcan.lib via #pragma.

ad3154

I know it's still in a draft state but here's some initial thoughts. Nothing major but I worry about them using an All Rights Reserved in their header file. We will have to do our due diligence to see if this can be added to the repo in the first place.

hardware_integration/src/ntcan_fifo_plugin.cpp

hardware_integration/include/isobus/hardware_integration/ntcan_fifo_plugin.hpp

ad3154 · 2023-03-03T04:46:07Z

hardware_integration/include/isobus/hardware_integration/NTCAN.h

+*                                                                         *
+* ntcan.h -- NTCAN-API procedure declarations and constant definitions    *
+*                                                                         *
+* Copyright (c) 1997 - 2022, esd electronics gmbh. All rights reserved.   *


ad3154 · 2023-03-03T04:51:58Z

hardware_integration/src/ntcan_fifo_plugin.cpp

+		if (NTCAN_SUCCESS == openResult)
+		{
+			ids = 0x20000000;
+			openResult = canIdRegionAdd(handle, 0 | NTCAN_20B_BASE, &ids);


0 | something is always something. Do you need the 0?

hardware_integration/src/ntcan_fifo_plugin.cpp

hardware_integration/include/isobus/hardware_integration/NTCAN.h

Y-Less · 2023-03-03T14:54:46Z

Thanks for the comments. Fortunately most of those seem fairly trivial. As for the main point - basic testing has at least confirmed it works.

Y-Less · 2023-03-04T04:25:58Z

So I'm not sure what the best way to deal with removing the explicit width types in, since I used those to match what the NTCAN API expects. If a function expects uint64_t I don't want to pass long long just in case... But maybe I should and just accept errors when they don't match. On the other hand the explicit width types are not exposed outside the plugin at all, and since the API itself has them this code isn't introducing any new incompatibilities that aren't introduced by the API itself. I've prefixed them with std:: anyway though.

0 | NTCAN_20B_BASE was me trying to be explicit. It is address 0 with the extended IDs flag, rather than just the flag alone. Yes it is pointless from a maths point of view, but I thought it was more helpful from an understanding point of view. I added a comment to this effect.

As for the inclusion of the library directly, I did that because the PCAN one did and I was just trying to emulate the existing style. However, since you need the driver installed to use this hardware backend anyway, and that includes the SDK with the latest version of the includes and libs, I don't think much is lost by removing it entirely and not worrying about the licenses at all. I can even rebase and purge the copies from git history.

- 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.