diff --git a/libuavcan/include/uavcan/protocol/dynamic_node_id_allocation_server.hpp b/libuavcan/include/uavcan/protocol/dynamic_node_id_allocation_server.hpp index f3ea8eb833..c8fde33897 100644 --- a/libuavcan/include/uavcan/protocol/dynamic_node_id_allocation_server.hpp +++ b/libuavcan/include/uavcan/protocol/dynamic_node_id_allocation_server.hpp @@ -137,6 +137,7 @@ private: protocol::dynamic_node_id::server::Entry entries_[Capacity]; Index last_index_; // Index zero always contains an empty entry + static IDynamicNodeIDStorageBackend::String getLastIndexKey() { return "log_last_index"; } static IDynamicNodeIDStorageBackend::String makeEntryKey(Index index, const char* postfix); int readEntryFromStorage(Index index, protocol::dynamic_node_id::server::Entry& out_entry); diff --git a/libuavcan/src/protocol/uc_dynamic_node_id_allocation_server.cpp b/libuavcan/src/protocol/uc_dynamic_node_id_allocation_server.cpp index a611bd5b6a..b7fac5fb34 100644 --- a/libuavcan/src/protocol/uc_dynamic_node_id_allocation_server.cpp +++ b/libuavcan/src/protocol/uc_dynamic_node_id_allocation_server.cpp @@ -123,7 +123,6 @@ int MarshallingStorageDecorator::get(const IDynamicNodeIDStorageBackend::String& } /* - * Per MISRA C++ recommendations, checking the inputs instead of relying solely on errno. * The value must contain only hexadecimal numbers. */ val.convertToLowerCaseASCII(); @@ -157,10 +156,10 @@ int MarshallingStorageDecorator::get(const IDynamicNodeIDStorageBackend::String& IDynamicNodeIDStorageBackend::String Log::makeEntryKey(Index index, const char* postfix) { IDynamicNodeIDStorageBackend::String str; - // "log0.foobar" + // "log0_foobar" str += "log"; str.appendFormatted("%d", int(index)); - str += "."; + str += "_"; str += postfix; return str; } @@ -187,6 +186,10 @@ int Log::readEntryFromStorage(Index index, protocol::dynamic_node_id::server::En { return -ErrFailure; } + if (node_id > NodeID::Max) + { + return -ErrFailure; + } out_entry.node_id = static_cast(node_id); return 0; @@ -227,12 +230,21 @@ int Log::init() // Reading max index { - IDynamicNodeIDStorageBackend::String key; - key = "log_last_index"; uint32_t value = 0; - if (io.get(key, value) < 0) + if (io.get(getLastIndexKey(), value) < 0) { - return -ErrFailure; + if (storage_.get(getLastIndexKey()).empty()) + { + // It appears like there's no log in the storage - initializing empty log then + last_index_ = 0; + UAVCAN_TRACE("dynamic_node_id_server_impl::Log", "Initializing empty log"); + return 0; + } + else + { + // There's some data in the storage, but it cannot be parsed - reporting an error + return -ErrFailure; + } } if (value >= Capacity) { @@ -251,28 +263,81 @@ int Log::init() } } + UAVCAN_TRACE("dynamic_node_id_server_impl::Log", "Restored %u log entries", unsigned(last_index_)); return 0; } int Log::append(const protocol::dynamic_node_id::server::Entry& entry) { - (void)entry; - return -1; + if ((last_index_ + 1) >= Capacity) + { + return -ErrLogic; + } + + // If next operations fail, we'll get a dangling entry, but it's absolutely OK. + int res = writeEntryToStorage(Index(last_index_ + 1), entry); + if (res < 0) + { + return res; + } + + // Updating the last index + MarshallingStorageDecorator io(storage_); + uint32_t new_last_index = last_index_ + 1U; + res = io.setAndGetBack(getLastIndexKey(), new_last_index); + if (res < 0) + { + return res; + } + if (new_last_index != last_index_ + 1U) + { + return -ErrFailure; + } + entries_[new_last_index] = entry; + last_index_ = Index(new_last_index); + + UAVCAN_TRACE("dynamic_node_id_server_impl::Log", "New entry, index %u, node ID %u, term %u", + unsigned(last_index_), unsigned(entry.node_id), unsigned(entry.term)); + return 0; } int Log::removeEntriesWhereIndexGreaterOrEqual(Index index) { - (void)index; - return -1; + UAVCAN_ASSERT(last_index_ < Capacity); + + if (((index) >= Capacity) || (index == 0)) + { + return -ErrLogic; + } + + MarshallingStorageDecorator io(storage_); + uint32_t new_last_index = index - 1U; + int res = io.setAndGetBack(getLastIndexKey(), new_last_index); + if (res < 0) + { + return res; + } + if (new_last_index != index - 1U) + { + return -ErrFailure; + } + UAVCAN_TRACE("dynamic_node_id_server_impl::Log", "Entries removed, last index %u --> %u", + unsigned(last_index_), unsigned(new_last_index)); + last_index_ = Index(new_last_index); + + // Removal operation leaves dangling entries in storage, it's OK + return 0; } const protocol::dynamic_node_id::server::Entry* Log::getEntryAtIndex(Index index) const { + UAVCAN_ASSERT(last_index_ < Capacity); return (index <= last_index_) ? &entries_[index] : NULL; } bool Log::isOtherLogUpToDate(Log::Index other_last_index, Term other_last_term) const { + UAVCAN_ASSERT(last_index_ < Capacity); // Terms are different - the one with higher term is more up-to-date if (other_last_term != entries_[last_index_].term) { diff --git a/libuavcan/test/protocol/dynamic_node_id_allocation_server.cpp b/libuavcan/test/protocol/dynamic_node_id_allocation_server.cpp index e509bbba7d..22c7d71a89 100644 --- a/libuavcan/test/protocol/dynamic_node_id_allocation_server.cpp +++ b/libuavcan/test/protocol/dynamic_node_id_allocation_server.cpp @@ -37,6 +37,7 @@ public: } }; + TEST(DynamicNodeIDAllocationServer, MarshallingStorageDecorator) { StorageBackend st; @@ -117,3 +118,91 @@ TEST(DynamicNodeIDAllocationServer, MarshallingStorageDecorator) key = "the_cake_is_a_lie"; ASSERT_GT(0, marshaler.get(key, array)); } + + +TEST(DynamicNodeIDAllocationServer, LogInitialization) +{ + // No log data in the storage - initializing empty log + { + StorageBackend storage; + uavcan::dynamic_node_id_server_impl::Log log(storage); + + ASSERT_LE(0, log.init()); + ASSERT_EQ(0, log.getLastIndex()); + ASSERT_EQ(0, log.getEntryAtIndex(0)->term); + ASSERT_EQ(0, log.getEntryAtIndex(0)->node_id); + ASSERT_EQ(uavcan::protocol::dynamic_node_id::server::Entry::FieldTypes::unique_id(), + log.getEntryAtIndex(0)->unique_id); + } + // Nonempty storage, one item + { + StorageBackend storage; + uavcan::dynamic_node_id_server_impl::Log log(storage); + + storage.set("log_last_index", "0"); + ASSERT_LE(-uavcan::ErrFailure, log.init()); // Expected one entry, none found + + storage.set("log0_term", "0"); + storage.set("log0_unique_id", "00000000000000000000000000000000"); + storage.set("log0_node_id", "0"); + ASSERT_LE(0, log.init()); // OK now + ASSERT_EQ(0, log.getLastIndex()); + ASSERT_EQ(0, log.getEntryAtIndex(0)->term); + } + // Nonempty storage, broken data + { + StorageBackend storage; + uavcan::dynamic_node_id_server_impl::Log log(storage); + + storage.set("log_last_index", "foobar"); + ASSERT_LE(-uavcan::ErrFailure, log.init()); // Bad value + + storage.set("log_last_index", "128"); + ASSERT_LE(-uavcan::ErrFailure, log.init()); // Bad value + + storage.set("log_last_index", "0"); + ASSERT_LE(-uavcan::ErrFailure, log.init()); // OK + + storage.set("log0_term", "0"); + storage.set("log0_unique_id", "00000000000000000000000000000000"); + storage.set("log0_node_id", "128"); // Bad value (127 max) + ASSERT_LE(-uavcan::ErrFailure, log.init()); // Failed + ASSERT_EQ(0, log.getLastIndex()); + ASSERT_EQ(0, log.getEntryAtIndex(0)->term); + } + // Nonempty storage, many items + { + StorageBackend storage; + uavcan::dynamic_node_id_server_impl::Log log(storage); + + storage.set("log_last_index", "1"); // 2 items - 0, 1 + storage.set("log0_term", "0"); + storage.set("log0_unique_id", "00000000000000000000000000000000"); + storage.set("log0_node_id", "0"); + storage.set("log1_term", "1"); + storage.set("log1_unique_id", "0123456789abcdef0123456789abcdef"); + storage.set("log1_node_id", "127"); + + ASSERT_LE(0, log.init()); // OK now + ASSERT_EQ(1, log.getLastIndex()); + + ASSERT_EQ(0, log.getEntryAtIndex(0)->term); + ASSERT_EQ(0, log.getEntryAtIndex(0)->node_id); + ASSERT_EQ(uavcan::protocol::dynamic_node_id::server::Entry::FieldTypes::unique_id(), + log.getEntryAtIndex(0)->unique_id); + + ASSERT_EQ(1, log.getEntryAtIndex(1)->term); + ASSERT_EQ(127, log.getEntryAtIndex(1)->node_id); + uavcan::protocol::dynamic_node_id::server::Entry::FieldTypes::unique_id uid; + uid[0] = 0x01; + uid[1] = 0x23; + uid[2] = 0x45; + uid[3] = 0x67; + uid[4] = 0x89; + uid[5] = 0xab; + uid[6] = 0xcd; + uid[7] = 0xef; + uavcan::copy(uid.begin(), uid.begin() + 8, uid.begin() + 8); + ASSERT_EQ(uid, log.getEntryAtIndex(1)->unique_id); + } +}