From 0d3d002e52cd9462b6090e0da27da90b8af3d3a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85smund=20Eek?= Date: Tue, 26 Mar 2019 10:29:28 +0100 Subject: [PATCH 1/3] Start of updating CAN ID header format to match v1.0 --- .gitignore | 9 ++++++++ canard.c | 40 ++++++++++----------------------- tests/test_rxerr.cpp | 53 ++++++++++++++++++++++---------------------- 3 files changed, 47 insertions(+), 55 deletions(-) diff --git a/.gitignore b/.gitignore index 0641d8d6..d0021352 100644 --- a/.gitignore +++ b/.gitignore @@ -34,6 +34,12 @@ # CMake build directory build/ cmake-build-*/ +CMakeFiles/ +cmake_install.cmake +CMakeCache.txt +Makefile +demo +run_tests # IDE and tools .idea @@ -49,3 +55,6 @@ dsdlc_generated/ # Pycache __pycache__/ + +# Ignore IDE folders +.vscode/ diff --git a/canard.c b/canard.c index 0f4e0954..22907e93 100644 --- a/canard.c +++ b/canard.c @@ -37,15 +37,14 @@ #define TRANSFER_TIMEOUT_USEC 2000000 #define TRANSFER_ID_BIT_LEN 5U -#define ANON_MSG_DATA_TYPE_ID_BIT_LEN 2U -#define SOURCE_ID_FROM_ID(x) ((uint8_t) (((x) >> 0U) & 0x7FU)) -#define SERVICE_NOT_MSG_FROM_ID(x) ((bool) (((x) >> 7U) & 0x1U)) -#define REQUEST_NOT_RESPONSE_FROM_ID(x) ((bool) (((x) >> 15U) & 0x1U)) +#define SOURCE_ID_FROM_ID(x) ((uint8_t) (((x) >> 1U) & 0x7FU)) +#define SERVICE_NOT_MSG_FROM_ID(x) ((bool) (((x) >> 25U) & 0x1U)) +#define REQUEST_NOT_RESPONSE_FROM_ID(x) ((bool) (((x) >> 26U) & 0x1U)) #define DEST_ID_FROM_ID(x) ((uint8_t) (((x) >> 8U) & 0x7FU)) -#define PRIORITY_FROM_ID(x) ((uint8_t) (((x) >> 24U) & 0x1FU)) +#define PRIORITY_FROM_ID(x) ((uint8_t) (((x) >> 26U) & 0x1FU)) #define MSG_TYPE_FROM_ID(x) ((uint16_t)(((x) >> 8U) & 0xFFFFU)) -#define SRV_TYPE_FROM_ID(x) ((uint8_t) (((x) >> 16U) & 0xFFU)) +#define SRV_TYPE_FROM_ID(x) ((uint8_t) (((x) >> 15U) & 0x1FFU)) #define MAKE_TRANSFER_DESCRIPTOR(data_type_id, transfer_type, src_node_id, dst_node_id) \ (((uint32_t)(data_type_id)) | (((uint32_t)(transfer_type)) << 16U) | \ @@ -151,6 +150,8 @@ int16_t canardBroadcast(CanardInstance* ins, uint32_t can_id = 0; uint16_t crc = 0xFFFFU; + can_id = ((uint32_t) priority << 26U) | ((uint32_t) data_type_id << 8U) | (uint32_t) canardGetLocalNodeID(ins); + if (canardGetLocalNodeID(ins) == 0) { if (payload_len > 7) @@ -158,22 +159,10 @@ int16_t canardBroadcast(CanardInstance* ins, return -CANARD_ERROR_NODE_ID_NOT_SET; } - static const uint16_t DTIDMask = (1U << ANON_MSG_DATA_TYPE_ID_BIT_LEN) - 1U; - - if ((data_type_id & DTIDMask) != data_type_id) - { - return -CANARD_ERROR_INVALID_ARGUMENT; - } - - // anonymous transfer, random discriminator - const uint16_t discriminator = (uint16_t)((crcAdd(0xFFFFU, payload, payload_len)) & 0x7FFEU); - can_id = ((uint32_t) priority << 24U) | ((uint32_t) discriminator << 9U) | - ((uint32_t) (data_type_id & DTIDMask) << 8U) | (uint32_t) canardGetLocalNodeID(ins); + can_id |= (1U << 24U); } else { - can_id = ((uint32_t) priority << 24U) | ((uint32_t) data_type_id << 8U) | (uint32_t) canardGetLocalNodeID(ins); - if (payload_len > 7) { crc = crcAddSignature(crc, data_type_signature); @@ -211,9 +200,9 @@ int16_t canardRequestOrRespond(CanardInstance* ins, return -CANARD_ERROR_NODE_ID_NOT_SET; } - const uint32_t can_id = ((uint32_t) priority << 24U) | ((uint32_t) data_type_id << 16U) | - ((uint32_t) kind << 15U) | ((uint32_t) destination_node_id << 8U) | - (1U << 7U) | (uint32_t) canardGetLocalNodeID(ins); + const uint32_t can_id = ((uint32_t) priority << 26U) | ((uint32_t) data_type_id << 15U) | + ((uint32_t) kind << 24U) | ((uint32_t) destination_node_id << 8U) | + (1U << 25U) | (uint32_t) canardGetLocalNodeID(ins); uint16_t crc = 0xFFFFU; if (payload_len > 7) @@ -1081,12 +1070,7 @@ CANARD_INTERNAL uint16_t extractDataType(uint32_t id) { if (extractTransferType(id) == CanardTransferTypeBroadcast) { - uint16_t dtid = MSG_TYPE_FROM_ID(id); - if (SOURCE_ID_FROM_ID(id) == CANARD_BROADCAST_NODE_ID) - { - dtid &= (1U << ANON_MSG_DATA_TYPE_ID_BIT_LEN) - 1U; - } - return dtid; + return (uint16_t) MSG_TYPE_FROM_ID(id); } else { diff --git a/tests/test_rxerr.cpp b/tests/test_rxerr.cpp index 9822c29d..4a1c6e25 100644 --- a/tests/test_rxerr.cpp +++ b/tests/test_rxerr.cpp @@ -26,35 +26,34 @@ #include //Independent implementation of ID layout from https://uavcan.org/Specification/4._CAN_bus_transport_layer/ -#define CONSTRUCT_PRIO(prio) (((prio) & 0x1F) << 24) -#define CONSTRUCT_MTID(mtid) (((mtid) & 0xFFFF) << 8) -#define CONSTRUCT_SID(sid) (((sid) & 0x7F)) -#define CONSTRUCT_DISC(disc) (((disc) & 0x3FFF) << 10) -#define CONSTRUCT_DISC_MTID(mtid) (((mtid) & 0x3) << 8) -#define CONSTRUCT_STID(stid) (((stid) & 0xFF) << 16) -#define CONSTRUCT_DID(did) (((did) & 0x7F) << 8) -#define CONSTRUCT_REQUEST(request) (((request) & 0x1) << 15) - -#define CONSTRUCT_SNM_BIT (1 << 7) - -#define CONSTRUCT_ANON_MSG_ID(prio, disc, mtid, sid) (CONSTRUCT_PRIO(prio) | \ - CONSTRUCT_DISC_MTID(mtid) | \ - CONSTRUCT_DISC(disc) | \ - CONSTRUCT_SID(sid) \ +#define CONSTRUCT_PRIO(prio) (((prio) & 0x7) << 26) +#define CONSTRUCT_SUBJECT_ID(subject_id) (((subject_id) & 0xFFFF) << 8) +#define CONSTRUCT_SOURCE_ID(source_id) (((source_id) & 0x7F) << 1) +#define CONSTRUCT_SERVICE_ID(service_tid) (((service_tid) & 0x1FF) << 15) +#define CONSTRUCT_DEST_ID(dest_id) (((dest_id) & 0x7F) << 8) +#define CONSTRUCT_REQUEST(request) (((request) & 0x1) << 24) + +#define CONSTRUCT_ANON_BIT (1 << 24) +#define CONSTRUCT_SNM_BIT (1 << 25) +#define CONSTRUCT_PV_BIT (1) + +#define CONSTRUCT_MSG_ID(prio, subject_id, source_id) (CONSTRUCT_PRIO(prio) | \ + CONSTRUCT_SUBJECT_ID(subject_id) | \ + CONSTRUCT_SOURCE_ID(source_id) | \ + CONSTRUCT_PV_BIT |\ CANARD_CAN_FRAME_EFF) -#define CONSTRUCT_MSG_ID(prio, mtid, sid) (CONSTRUCT_PRIO(prio) | \ - CONSTRUCT_MTID(mtid) | \ - CONSTRUCT_SID(sid) | \ - CANARD_CAN_FRAME_EFF) - -#define CONSTRUCT_SVC_ID(prio, stid, request, did, sid) (CONSTRUCT_PRIO(prio) | \ - CONSTRUCT_STID(stid) | \ - CONSTRUCT_REQUEST(request) | \ - CONSTRUCT_DID(did) | \ - CONSTRUCT_SID(sid) | \ - CONSTRUCT_SNM_BIT | \ - CANARD_CAN_FRAME_EFF) +#define CONSTRUCT_ANON_MSG_ID(prio, subject_id, source_id) (CONSTRUCT_ANON_BIT | \ + CONSTRUCT_MSG_ID(prio, subject_id, source_id)) + +#define CONSTRUCT_SVC_ID(prio, service_id, request, dest_id, source_id) (CONSTRUCT_PRIO(prio) | \ + CONSTRUCT_SNM_BIT | \ + CONSTRUCT_REQUEST(request) | \ + CONSTRUCT_SERVICE_ID(service_id) | \ + CONSTRUCT_DEST_ID(dest_id) | \ + CONSTRUCT_SOURCE_ID(source_id) | \ + CONSTRUCT_PV_BIT |\ + CANARD_CAN_FRAME_EFF) #define CONSTRUCT_START(start) (((start) & 0x1) << 7) #define CONSTRUCT_END(end) (((end) & 0x1) << 6) From f780ae07dd7334037c195e331a19df0261b2c4ca Mon Sep 17 00:00:00 2001 From: aasmune Date: Tue, 26 Mar 2019 22:19:17 +0100 Subject: [PATCH 2/3] Review changes --- DESIGN.md | 4 ++-- canard.c | 29 ++++++++++++++++------------- canard.h | 8 +++++--- tests/test_rxerr.cpp | 2 +- 4 files changed, 24 insertions(+), 19 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index cbf1aa48..79b24b07 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -255,8 +255,8 @@ Note that the proposed API does not make any assumptions about the CAN driver in A rough draft of the API definitions is provided below: ```c -#define CANARD_BROADCAST_NODE_ID 0 -#define CANARD_MIN_NODE_ID 1 +#define CANARD_BROADCAST_NODE_ID 255 +#define CANARD_MIN_NODE_ID 0 #define CANARD_MAX_NODE_ID 127 #define CANARD_MEM_BLOCK_SIZE 32 diff --git a/canard.c b/canard.c index 22907e93..0352e8f0 100644 --- a/canard.c +++ b/canard.c @@ -40,14 +40,14 @@ #define SOURCE_ID_FROM_ID(x) ((uint8_t) (((x) >> 1U) & 0x7FU)) #define SERVICE_NOT_MSG_FROM_ID(x) ((bool) (((x) >> 25U) & 0x1U)) -#define REQUEST_NOT_RESPONSE_FROM_ID(x) ((bool) (((x) >> 26U) & 0x1U)) +#define REQUEST_NOT_RESPONSE_FROM_ID(x) ((bool) (((x) >> 24U) & 0x1U)) #define DEST_ID_FROM_ID(x) ((uint8_t) (((x) >> 8U) & 0x7FU)) #define PRIORITY_FROM_ID(x) ((uint8_t) (((x) >> 26U) & 0x1FU)) -#define MSG_TYPE_FROM_ID(x) ((uint16_t)(((x) >> 8U) & 0xFFFFU)) +#define SUBJECT_TYPE_FROM_ID(x) ((uint16_t)(((x) >> 8U) & 0x7FFFU)) #define SRV_TYPE_FROM_ID(x) ((uint8_t) (((x) >> 15U) & 0x1FFU)) -#define MAKE_TRANSFER_DESCRIPTOR(data_type_id, transfer_type, src_node_id, dst_node_id) \ - (((uint32_t)(data_type_id)) | (((uint32_t)(transfer_type)) << 16U) | \ +#define MAKE_TRANSFER_DESCRIPTOR(port_id, transfer_type, src_node_id, dst_node_id) \ + (((uint32_t)(port_id)) | (((uint32_t)(transfer_type)) << 16U) | \ (((uint32_t)(src_node_id)) << 18U) | (((uint32_t)(dst_node_id)) << 25U)) #define TRANSFER_ID_FROM_TAIL_BYTE(x) ((uint8_t)((x) & 0x1FU)) @@ -109,20 +109,21 @@ void* canardGetUserReference(CanardInstance* ins) return ins->user_reference; } -void canardSetLocalNodeID(CanardInstance* ins, uint8_t self_node_id) +int16_t canardSetLocalNodeID(CanardInstance* ins, uint8_t self_node_id) { CANARD_ASSERT(ins != NULL); if ((ins->node_id == CANARD_BROADCAST_NODE_ID) && - (self_node_id >= CANARD_MIN_NODE_ID) && (self_node_id <= CANARD_MAX_NODE_ID)) { ins->node_id = self_node_id; } else { - CANARD_ASSERT(false); + return - CANARD_ERROR_INVALID_ARGUMENT; } + + return CANARD_OK; } uint8_t canardGetLocalNodeID(const CanardInstance* ins) @@ -150,16 +151,17 @@ int16_t canardBroadcast(CanardInstance* ins, uint32_t can_id = 0; uint16_t crc = 0xFFFFU; - can_id = ((uint32_t) priority << 26U) | ((uint32_t) data_type_id << 8U) | (uint32_t) canardGetLocalNodeID(ins); + can_id = ((uint32_t) priority << 26U) | ((uint32_t) data_type_id << 8U) | ((uint32_t) canardGetLocalNodeID(ins) << 1U); - if (canardGetLocalNodeID(ins) == 0) + if (canardGetLocalNodeID(ins) == CANARD_BROADCAST_NODE_ID) // Anonymous message transfer { if (payload_len > 7) { return -CANARD_ERROR_NODE_ID_NOT_SET; } - can_id |= (1U << 24U); + uint8_t pseudo_id = crcAdd(0xFFFFU, payload, payload_len) & 0x7FU; + can_id |= (1U << 24U) | ((uint32_t) pseudo_id << 1U); } else { @@ -195,14 +197,15 @@ int16_t canardRequestOrRespond(CanardInstance* ins, { return -CANARD_ERROR_INVALID_ARGUMENT; } - if (canardGetLocalNodeID(ins) == 0) + if (canardGetLocalNodeID(ins) == CANARD_BROADCAST_NODE_ID) { + // Service transfer not supported for anonymous nodes return -CANARD_ERROR_NODE_ID_NOT_SET; } const uint32_t can_id = ((uint32_t) priority << 26U) | ((uint32_t) data_type_id << 15U) | ((uint32_t) kind << 24U) | ((uint32_t) destination_node_id << 8U) | - (1U << 25U) | (uint32_t) canardGetLocalNodeID(ins); + (1U << 25U) | ((uint32_t) canardGetLocalNodeID(ins) << 1U); uint16_t crc = 0xFFFFU; if (payload_len > 7) @@ -1070,7 +1073,7 @@ CANARD_INTERNAL uint16_t extractDataType(uint32_t id) { if (extractTransferType(id) == CanardTransferTypeBroadcast) { - return (uint16_t) MSG_TYPE_FROM_ID(id); + return (uint16_t) SUBJECT_TYPE_FROM_ID(id); } else { diff --git a/canard.h b/canard.h index 58532d8f..ab37d72e 100644 --- a/canard.h +++ b/canard.h @@ -87,8 +87,8 @@ extern "C" { #define CANARD_CAN_FRAME_MAX_DATA_LEN 8U /// Node ID values. Refer to the specification for more info. -#define CANARD_BROADCAST_NODE_ID 0 -#define CANARD_MIN_NODE_ID 1 +#define CANARD_BROADCAST_NODE_ID 255 +#define CANARD_MIN_NODE_ID 0 #define CANARD_MAX_NODE_ID 127 /// Refer to the type CanardRxTransfer @@ -346,8 +346,10 @@ void* canardGetUserReference(CanardInstance* ins); /** * Assigns a new node ID value to the current node. * Node ID can be assigned only once. + * + * Returns CANARD_OK if success, or negative error code if node ID is outside valid range. */ -void canardSetLocalNodeID(CanardInstance* ins, +int16_t canardSetLocalNodeID(CanardInstance* ins, uint8_t self_node_id); /** diff --git a/tests/test_rxerr.cpp b/tests/test_rxerr.cpp index 4a1c6e25..3922feec 100644 --- a/tests/test_rxerr.cpp +++ b/tests/test_rxerr.cpp @@ -29,7 +29,7 @@ #define CONSTRUCT_PRIO(prio) (((prio) & 0x7) << 26) #define CONSTRUCT_SUBJECT_ID(subject_id) (((subject_id) & 0xFFFF) << 8) #define CONSTRUCT_SOURCE_ID(source_id) (((source_id) & 0x7F) << 1) -#define CONSTRUCT_SERVICE_ID(service_tid) (((service_tid) & 0x1FF) << 15) +#define CONSTRUCT_SERVICE_ID(service_id) (((service_id) & 0x1FF) << 15) #define CONSTRUCT_DEST_ID(dest_id) (((dest_id) & 0x7F) << 8) #define CONSTRUCT_REQUEST(request) (((request) & 0x1) << 24) From 55b73451c79a2193b608e19e95926f209ef7f5e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85smund=20Eek?= Date: Wed, 27 Mar 2019 20:42:28 +0100 Subject: [PATCH 3/3] More review changes --- .gitignore | 7 +------ .travis.yml | 7 ++++--- DESIGN.md | 2 +- canard.c | 10 ++++++---- canard.h | 6 +++--- 5 files changed, 15 insertions(+), 17 deletions(-) diff --git a/.gitignore b/.gitignore index d0021352..dd396236 100644 --- a/.gitignore +++ b/.gitignore @@ -34,12 +34,7 @@ # CMake build directory build/ cmake-build-*/ -CMakeFiles/ -cmake_install.cmake -CMakeCache.txt -Makefile -demo -run_tests +build-avr/ # IDE and tools .idea diff --git a/.travis.yml b/.travis.yml index 04a3b84f..6e3acf82 100644 --- a/.travis.yml +++ b/.travis.yml @@ -18,7 +18,8 @@ matrix: - gcc-7-multilib - linux-libc-dev:i386 script: - - CC=gcc-7 && CXX=g++-7 && cd tests/ && cmake . && make + - rm -rf tests/build && mkdir tests/build/ && cd tests/build/ + - CC=gcc-7 && CXX=g++-7 && cmake .. && make - ./run_tests --rng-seed time # @@ -37,9 +38,9 @@ matrix: - linux-libc-dev:i386 - libc6-dev-i386 script: + - rm -rf tests/build && mkdir tests/build/ && cd tests/build/ - clang++-5.0 -E -x c++ - -v < /dev/null # Print the Clang configuration for troubleshooting purposes - - cd tests/ - - cmake -DCMAKE_C_COMPILER=clang-5.0 -DCMAKE_CXX_COMPILER=clang++-5.0 . + - cmake -DCMAKE_C_COMPILER=clang-5.0 -DCMAKE_CXX_COMPILER=clang++-5.0 .. - make - ./run_tests --rng-seed time diff --git a/DESIGN.md b/DESIGN.md index 79b24b07..1bd9a227 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -142,7 +142,7 @@ Few things to note: Value of the field dtid_tt_snid_dnid can be computed with the following helper macro: ```c -#define CANARD_MAKE_TRANSFER_DESCRIPTOR(data_type_id, transfer_type, \ +#define CANARD_MAKE_SESSION_SPECIFIER(data_type_id, transfer_type, \ src_node_id, dst_node_id) \ ((data_type_id) | ((transfer_type) << 16) | \ ((src_node_id) << 18) | ((dst_node_id) << 25)) diff --git a/canard.c b/canard.c index 0352e8f0..86ae4d40 100644 --- a/canard.c +++ b/canard.c @@ -42,11 +42,11 @@ #define SERVICE_NOT_MSG_FROM_ID(x) ((bool) (((x) >> 25U) & 0x1U)) #define REQUEST_NOT_RESPONSE_FROM_ID(x) ((bool) (((x) >> 24U) & 0x1U)) #define DEST_ID_FROM_ID(x) ((uint8_t) (((x) >> 8U) & 0x7FU)) -#define PRIORITY_FROM_ID(x) ((uint8_t) (((x) >> 26U) & 0x1FU)) +#define PRIORITY_FROM_ID(x) ((uint8_t) (((x) >> 26U) & 0x7U)) #define SUBJECT_TYPE_FROM_ID(x) ((uint16_t)(((x) >> 8U) & 0x7FFFU)) #define SRV_TYPE_FROM_ID(x) ((uint8_t) (((x) >> 15U) & 0x1FFU)) -#define MAKE_TRANSFER_DESCRIPTOR(port_id, transfer_type, src_node_id, dst_node_id) \ +#define MAKE_SESSION_SPECIFIER(port_id, transfer_type, src_node_id, dst_node_id) \ (((uint32_t)(port_id)) | (((uint32_t)(transfer_type)) << 16U) | \ (((uint32_t)(src_node_id)) << 18U) | (((uint32_t)(dst_node_id)) << 25U)) @@ -151,7 +151,7 @@ int16_t canardBroadcast(CanardInstance* ins, uint32_t can_id = 0; uint16_t crc = 0xFFFFU; - can_id = ((uint32_t) priority << 26U) | ((uint32_t) data_type_id << 8U) | ((uint32_t) canardGetLocalNodeID(ins) << 1U); + can_id = ((uint32_t) priority << 26U) | ((uint32_t) data_type_id << 8U); if (canardGetLocalNodeID(ins) == CANARD_BROADCAST_NODE_ID) // Anonymous message transfer { @@ -170,6 +170,8 @@ int16_t canardBroadcast(CanardInstance* ins, crc = crcAddSignature(crc, data_type_signature); crc = crcAdd(crc, payload, payload_len); } + + can_id |= ((uint32_t) (canardGetLocalNodeID(ins) & 0x7FU) << 1U); } const int16_t result = enqueueTxFrames(ins, can_id, inout_transfer_id, crc, payload, payload_len); @@ -267,7 +269,7 @@ int16_t canardHandleRxFrame(CanardInstance* ins, const CanardCANFrame* frame, ui const uint8_t source_node_id = SOURCE_ID_FROM_ID(frame->id); const uint16_t data_type_id = extractDataType(frame->id); const uint32_t transfer_descriptor = - MAKE_TRANSFER_DESCRIPTOR(data_type_id, transfer_type, source_node_id, destination_node_id); + MAKE_SESSION_SPECIFIER(data_type_id, transfer_type, source_node_id, destination_node_id); const uint8_t tail_byte = frame->data[frame->data_len - 1]; diff --git a/canard.h b/canard.h index ab37d72e..d8bc7554 100644 --- a/canard.h +++ b/canard.h @@ -347,14 +347,14 @@ void* canardGetUserReference(CanardInstance* ins); * Assigns a new node ID value to the current node. * Node ID can be assigned only once. * - * Returns CANARD_OK if success, or negative error code if node ID is outside valid range. + * Returns CANARD_OK if success, or negative error code if node ID is outside valid range or already configured. */ int16_t canardSetLocalNodeID(CanardInstance* ins, - uint8_t self_node_id); + uint8_t self_node_id); /** * Returns node ID of the local node. - * Returns zero (broadcast) if the node ID is not set, i.e. if the local node is anonymous. + * Returns 255 (broadcast) if the node ID is not set, i.e. if the local node is anonymous. */ uint8_t canardGetLocalNodeID(const CanardInstance* ins);