From f780ae07dd7334037c195e331a19df0261b2c4ca Mon Sep 17 00:00:00 2001 From: aasmune Date: Tue, 26 Mar 2019 22:19:17 +0100 Subject: [PATCH] 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)