Review changes

OpenCyphal · Mar 26, 2019 · f780ae0 · f780ae0
1 parent 0d3d002
commit f780ae0
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 19 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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)