Refactoring: Frame Index field size increased, Trnasfer ID field size reduced. NodeID class added, Frame class rewritten with stricter runtime checks. All tests were updated accordingly.

libuavcan/include/uavcan/internal/transport/dispatcher.hpp

@@ -49,13 +49,13 @@ class Dispatcher : Noncopyable
     ListenerRegister lsrv_req_;
     ListenerRegister lsrv_resp_;
 
-    const uint8_t self_node_id_;
+    const NodeID self_node_id_;
 
     void handleFrame(const CanRxFrame& can_frame);
 
 public:
     Dispatcher(ICanDriver* driver, IAllocator* allocator, ISystemClock* sysclock, IOutgoingTransferRegistry* otr,
-               uint8_t self_node_id)
+               NodeID self_node_id)
     : canio_(driver, allocator, sysclock)
     , sysclock_(sysclock)
     , outgoing_transfer_reg_(otr)
@@ -87,7 +87,7 @@ public:
 
     IOutgoingTransferRegistry* getOutgoingTransferRegistry() { return outgoing_transfer_reg_; }
 
-    uint8_t getSelfNodeID() const { return self_node_id_; }
+    NodeID getSelfNodeID() const { return self_node_id_; }
 };
 
 }

libuavcan/include/uavcan/internal/transport/outgoing_transfer_registry.hpp

@@ -18,22 +18,20 @@ class OutgoingTransferRegistryKey
 {
     uint16_t data_type_id_;
     uint8_t transfer_type_;
-    uint8_t destination_node_id_;  ///< Not applicable for message broadcasting
+    NodeID destination_node_id_;  ///< Not applicable for message broadcasting
 
 public:
     OutgoingTransferRegistryKey()
     : data_type_id_(0xFFFF)
     , transfer_type_(0xFF)
-    , destination_node_id_(NODE_ID_INVALID)
     { }
 
-    OutgoingTransferRegistryKey(uint16_t data_type_id, TransferType transfer_type, uint8_t destination_node_id)
+    OutgoingTransferRegistryKey(uint16_t data_type_id, TransferType transfer_type, NodeID destination_node_id)
     : data_type_id_(data_type_id)
     , transfer_type_(transfer_type)
     , destination_node_id_(destination_node_id)
     {
-        assert(destination_node_id != NODE_ID_INVALID);
-        assert((transfer_type == TRANSFER_TYPE_MESSAGE_BROADCAST) == (destination_node_id == NODE_ID_BROADCAST));
+        assert((transfer_type == TRANSFER_TYPE_MESSAGE_BROADCAST) == destination_node_id.isBroadcast());
 
         /* Service response transfers must use the same Transfer ID as matching service request transfer,
          * so this registry is not applicable for service response transfers at all.

libuavcan/include/uavcan/internal/transport/transfer.hpp

@@ -12,13 +12,6 @@
 namespace uavcan
 {
 
-enum NodeIDConstants
-{
-    NODE_ID_BROADCAST = 0,
-    NODE_ID_MAX       = 127,
-    NODE_ID_INVALID   = 255
-};
-
 enum TransferType
 {
     TRANSFER_TYPE_SERVICE_RESPONSE  = 0,
@@ -29,12 +22,46 @@ enum TransferType
 };
 
 
+class NodeID
+{
+    enum
+    {
+        VALUE_BROADCAST = 0,
+        VALUE_INVALID = 0xFF
+    };
+    uint8_t value_;
+
+public:
+    enum { BITLEN = 7 };
+    enum { MAX = (1 << BITLEN) - 1 };
+
+    static const NodeID BROADCAST;
+
+    NodeID() : value_(VALUE_INVALID) { }
+
+    NodeID(uint8_t value)
+    : value_(value)
+    {
+        assert(isValid());
+    }
+
+    uint8_t get() const { return value_; }
+
+    bool isValid() const     { return value_ <= MAX; }
+    bool isBroadcast() const { return value_ == VALUE_BROADCAST; }
+    bool isUnicast() const   { return (value_ <= MAX) && (value_ != VALUE_BROADCAST); }
+
+    bool operator!=(NodeID rhs) const { return !operator==(rhs); }
+    bool operator==(NodeID rhs) const { return value_ == rhs.value_; }
+};
+
+
 class TransferID
 {
     uint8_t value_;
 
 public:
-    enum { BITLEN = 4 };
+    enum { BITLEN = 3 };
     enum { MAX = (1 << BITLEN) - 1 };
 
     TransferID()
@@ -69,94 +96,114 @@ public:
 };
 
 
-struct Frame
+class Frame
 {
-    enum { DATA_TYPE_ID_MAX = 1023 };
-    enum { FRAME_INDEX_MAX = 31 };
-    enum { PAYLOAD_LEN_MAX = 8 };
+    uint8_t payload_[sizeof(CanFrame::data)];
+    TransferType transfer_type_;
+    uint_fast16_t data_type_id_;
+    uint_fast8_t payload_len_;
+    NodeID src_node_id_;
+    NodeID dst_node_id_;
+    uint_fast8_t frame_index_;
+    TransferID transfer_id_;
+    bool last_frame_;
 
-    uint8_t payload[PAYLOAD_LEN_MAX];
-    TransferType transfer_type;
-    uint_fast16_t data_type_id;
-    uint_fast8_t payload_len;
-    uint_fast8_t source_node_id;
-    uint_fast8_t frame_index;
-    TransferID transfer_id;
-    bool last_frame;
+public:
+    enum { DATA_TYPE_ID_MAX = 1023 };
+    enum { FRAME_INDEX_MAX = 62 };        // 63 (or 0b111111) is reserved
 
     Frame()
-    : transfer_type(TransferType(0))
-    , data_type_id(0)
-    , payload_len(0)
-    , source_node_id(0)
-    , frame_index(0)
-    , transfer_id(0)
-    , last_frame(false)
+    : transfer_type_(TransferType(NUM_TRANSFER_TYPES))  // That is invalid value
+    , data_type_id_(0)
+    , payload_len_(0)
+    , frame_index_(0)
+    , transfer_id_(0)
+    , last_frame_(false)
+    { }
+
+    Frame(uint_fast16_t data_type_id, TransferType transfer_type, NodeID src_node_id, NodeID dst_node_id,
+          uint_fast8_t frame_index, TransferID transfer_id, bool last_frame = false)
+    : transfer_type_(transfer_type)
+    , data_type_id_(data_type_id)
+    , payload_len_(0)
+    , src_node_id_(src_node_id)
+    , dst_node_id_(dst_node_id)
+    , frame_index_(frame_index)
+    , transfer_id_(transfer_id)
+    , last_frame_(last_frame)
     {
-        std::fill(payload, payload + sizeof(payload), 0);
+        assert((transfer_type == TRANSFER_TYPE_MESSAGE_BROADCAST) == dst_node_id.isBroadcast());
+        assert(data_type_id <= DATA_TYPE_ID_MAX);
+        assert(src_node_id != dst_node_id);
+        assert(frame_index <= FRAME_INDEX_MAX);
     }
 
-    Frame(const uint8_t* payload, uint_fast8_t payload_len, uint_fast16_t data_type_id, TransferType transfer_type,
-          uint_fast8_t source_node_id, uint_fast8_t frame_index, TransferID transfer_id, bool last_frame)
-    : transfer_type(transfer_type)
-    , data_type_id(data_type_id)
-    , payload_len(payload_len)
-    , source_node_id(source_node_id)
-    , frame_index(frame_index)
-    , transfer_id(transfer_id)
-    , last_frame(last_frame)
-    {
-        assert(data_type_id <= DATA_TYPE_ID_MAX);
-        assert(source_node_id <= NODE_ID_MAX);
-        assert(frame_index <= FRAME_INDEX_MAX);
-        assert(payload && payload_len <= sizeof(payload));
-        std::copy(payload, payload + payload_len, this->payload);
-    }
+    int getMaxPayloadLen() const;
+    int setPayload(const uint8_t* data, int len);
+
+    int getPayloadLen() const { return payload_len_; }
+    const uint8_t* getPayloadPtr() const { return payload_; }
+
+    TransferType getTransferType() const { return transfer_type_; }
+    uint_fast16_t getDataTypeID()  const { return data_type_id_; }
+    NodeID getSrcNodeID()          const { return src_node_id_; }
+    NodeID getDstNodeID()          const { return dst_node_id_; }
+    TransferID getTransferID()     const { return transfer_id_; }
+    uint_fast8_t getFrameIndex()   const { return frame_index_; }
+    bool isLastFrame()             const { return last_frame_; }
+
+    void makeLast() { last_frame_ = true; }
+
+    bool isFirstFrame() const { return frame_index_ == 0; }
 
     bool parse(const CanFrame& can_frame);
+    bool compile(CanFrame& can_frame) const;
 
-    CanFrame compile() const;
+    bool isValid() const;
 
     bool operator!=(const Frame& rhs) const { return !operator==(rhs); }
-    bool operator==(const Frame& rhs) const
-    {
-        return
-            (transfer_type == rhs.transfer_type) &&
-            (data_type_id == rhs.data_type_id) &&
-            (source_node_id == rhs.source_node_id) &&
-            (frame_index == rhs.frame_index) &&
-            (transfer_id == rhs.transfer_id) &&
-            (last_frame == rhs.last_frame) &&
-            (payload_len == rhs.payload_len) &&
-            std::equal(payload, payload + payload_len, rhs.payload);
-    }
+    bool operator==(const Frame& rhs) const;
 
     std::string toString() const;
 };
 
 
-struct RxFrame : public Frame
+class RxFrame : public Frame
 {
-    uint_fast64_t ts_monotonic;
-    uint_fast64_t ts_utc;
-    uint_fast8_t iface_index;
+    uint64_t ts_monotonic_;
+    uint64_t ts_utc_;
+    uint8_t iface_index_;
 
+public:
     RxFrame()
-    : ts_monotonic(0)
-    , ts_utc(0)
-    , iface_index(0)
+    : ts_monotonic_(0)
+    , ts_utc_(0)
+    , iface_index_(0)
     { }
 
+    RxFrame(const Frame& frame, uint64_t ts_monotonic, uint64_t ts_utc, uint8_t iface_index)
+    : ts_monotonic_(ts_monotonic)
+    , ts_utc_(ts_utc)
+    , iface_index_(iface_index)
+    {
+        *static_cast<Frame*>(this) = frame;
+    }
+
     bool parse(const CanRxFrame& can_frame)
     {
         if (!Frame::parse(can_frame))
             return false;
-        ts_monotonic = can_frame.ts_monotonic;
-        ts_utc = can_frame.ts_utc;
-        iface_index = can_frame.iface_index;
+        ts_monotonic_ = can_frame.ts_monotonic;
+        ts_utc_ = can_frame.ts_utc;
+        iface_index_ = can_frame.iface_index;
         return true;
     }
 
+    uint64_t getMonotonicTimestamp() const { return ts_monotonic_; }
+    uint64_t getUtcTimestamp()       const { return ts_utc_; }
+
+    uint8_t getIfaceIndex() const { return iface_index_; }
+
     std::string toString() const;
 };
 

libuavcan/include/uavcan/internal/transport/transfer_buffer.hpp

@@ -32,22 +32,20 @@ public:
  */
 class TransferBufferManagerKey
 {
-    uint8_t node_id_;
+    NodeID node_id_;
     uint8_t transfer_type_;
 
 public:
     TransferBufferManagerKey()
-    : node_id_(NODE_ID_INVALID)
-    , transfer_type_(TransferType(0))
+    : transfer_type_(TransferType(0))
     {
         assert(isEmpty());
     }
 
-    TransferBufferManagerKey(uint8_t node_id, TransferType ttype)
+    TransferBufferManagerKey(NodeID node_id, TransferType ttype)
     : node_id_(node_id)
     , transfer_type_(ttype)
     {
-        assert(node_id <= NODE_ID_MAX && node_id != NODE_ID_INVALID);
         assert(!isEmpty());
     }
 
@@ -56,9 +54,9 @@ public:
         return node_id_ == rhs.node_id_ && transfer_type_ == rhs.transfer_type_;
     }
 
-    bool isEmpty() const { return node_id_ == NODE_ID_INVALID; }
+    bool isEmpty() const { return !node_id_.isValid(); }
 
-    uint8_t getNodeID() const { return node_id_; }
+    NodeID getNodeID() const { return node_id_; }
     TransferType getTransferType() const { return TransferType(transfer_type_); }
 
     std::string toString() const;

libuavcan/include/uavcan/internal/transport/transfer_listener.hpp

@@ -25,16 +25,16 @@ class IncomingTransfer
     uint64_t ts_utc_;
     TransferType transfer_type_;
     TransferID transfer_id_;
-    uint8_t source_node_id_;
+    NodeID src_node_id_;
 
 protected:
     IncomingTransfer(uint64_t ts_monotonic, uint64_t ts_utc, TransferType transfer_type,
-                     TransferID transfer_id, uint8_t source_node_id)
+                     TransferID transfer_id, NodeID source_node_id)
     : ts_monotonic_(ts_monotonic)
     , ts_utc_(ts_utc)
     , transfer_type_(transfer_type)
     , transfer_id_(transfer_id)
-    , source_node_id_(source_node_id)
+    , src_node_id_(source_node_id)
     { }
 
 public:
@@ -54,7 +54,7 @@ public:
     uint64_t getUtcTimestamp()       const { return ts_utc_; }
     TransferType getTransferType()   const { return transfer_type_; }
     TransferID getTransferID()       const { return transfer_id_; }
-    uint8_t getSourceNodeID()        const { return source_node_id_; }
+    NodeID getSrcNodeID()            const { return src_node_id_; }
 };
 
 /**
@@ -65,7 +65,7 @@ class SingleFrameIncomingTransfer : public IncomingTransfer
     const uint8_t* const payload_;
     const uint8_t payload_len_;
 public:
-    SingleFrameIncomingTransfer(const RxFrame& frm, const uint8_t* payload, unsigned int payload_len);
+    SingleFrameIncomingTransfer(const RxFrame& frm);
     int read(unsigned int offset, uint8_t* data, unsigned int len) const;
 };
 
@@ -125,16 +125,12 @@ class TransferListener : public TransferListenerBase, Noncopyable
 
     void handleFrame(const RxFrame& frame)
     {
-        const TransferBufferManagerKey key(frame.source_node_id, frame.transfer_type);
+        const TransferBufferManagerKey key(frame.getSrcNodeID(), frame.getTransferType());
 
         TransferReceiver* recv = receivers_.access(key);
         if (recv == NULL)
         {
-            /* Adding new registrations mid-transfer (i.e. not upon first frame reception) is not only
-             * pointless, but plain wrong: non-first frames do not carry address information, so we
-             * have no chance to detect whether a transfer is addressed to our node or not.
-             */
-            if (frame.frame_index != 0)
+            if (!frame.isFirstFrame())
                 return;
 
             TransferReceiver new_recv;

libuavcan/include/uavcan/internal/transport/transfer_receiver.hpp

@@ -68,8 +68,6 @@ public:
     uint16_t getLastTransferCrc() const { return this_transfer_crc_; }
 
     uint32_t getInterval() const { return transfer_interval_; }
-
-    static bool extractSingleFrameTransferPayload(const RxFrame& frame, uint8_t* out_data, unsigned int& out_len);
 };
 #pragma pack(pop)