Skip to content

Commit

Permalink
[API] remove the PanId struct
Browse files Browse the repository at this point in the history
It's an overkill to define a dedicated class/struct for a simple
`uint16_t` just for pretty print to json in HEX string.
  • Loading branch information
wgtdkp committed May 1, 2024
1 parent 4543bd1 commit ef31c91
Show file tree
Hide file tree
Showing 19 changed files with 115 additions and 140 deletions.
11 changes: 9 additions & 2 deletions include/commissioner/error.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -264,14 +264,21 @@ inline bool operator!=(const ErrorCode &aErrorCode, const Error &aError)
return !(aErrorCode == aError);
}

std::string ErrorCodeToString(ErrorCode code);

/**
* Allows pretty-print in unit tests.
*
* See https://google.github.io/googletest/advanced.html#teaching-googletest-how-to-print-your-values
*/
inline void PrintTo(const Error &error, std::ostream *os)
inline void PrintTo(const Error &aError, std::ostream *os)
{
*os << aError.ToString();
}

inline void PrintTo(ErrorCode aErrorCode, std::ostream *os)
{
*os << error.ToString();
*os << ErrorCodeToString(aErrorCode);
}

} // namespace commissioner
Expand Down
22 changes: 1 addition & 21 deletions include/commissioner/network_data.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -249,26 +249,6 @@ enum SecurityPolicyFlags
kSecurityPolicyMask_VR = 1 << 5 | 1 << 6 | 1 << 7, /// Protocol version
};

/**
* A PAN identifier.
*/
struct PanId
{
static constexpr uint64_t kEmptyPanId = 0;

uint16_t mValue;
explicit PanId(uint16_t aValue);
PanId(const PanId &aOther) = default;
PanId();

PanId &operator=(const PanId &aValue) = default;
PanId &operator=(uint16_t aValue);
explicit operator uint16_t() const;
explicit operator std::string() const;

Error FromHex(const std::string &aInput);
};

/**
* @brief The Active Operational Dataset of the Thread Network Data.
*
Expand All @@ -289,7 +269,7 @@ struct ActiveOperationalDataset
ByteArray mMeshLocalPrefix;
ByteArray mNetworkMasterKey;
std::string mNetworkName;
PanId mPanId;
uint16_t mPanId;
ByteArray mPSKc;
SecurityPolicy mSecurityPolicy;

Expand Down
29 changes: 7 additions & 22 deletions src/app/cli/interpreter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ namespace ot {
namespace commissioner {

using ot::commissioner::persistent_storage::Network;
using ot::commissioner::utils::Hex;
using ot::commissioner::utils::ParseInteger;

namespace {
/**
Expand Down Expand Up @@ -349,23 +351,6 @@ template <typename T> static std::string ToHex(T aInteger)
return "0x" + utils::Hex(utils::Encode(aInteger));
};

template <typename T> static Error ParseInteger(T &aInteger, const std::string &aStr)
{
Error error;
uint64_t integer;
char *endPtr = nullptr;

integer = strtoull(aStr.c_str(), &endPtr, 0);

VerifyOrExit(endPtr != nullptr && endPtr > aStr.c_str(),
error = ERROR_INVALID_ARGS("{} is not a valid integer", aStr));

aInteger = integer;

exit:
return error;
}

static inline std::string ToLower(const std::string &aStr)
{
return utils::ToLower(aStr);
Expand Down Expand Up @@ -2100,7 +2085,7 @@ Interpreter::Value Interpreter::ProcessOpDatasetJob(CommissionerAppPtr &aCommiss
}
else if (CaseInsensitiveEqual(aExpr[2], "panid"))
{
PanId panid;
uint16_t panid;
if (isSet)
{
uint32_t delay;
Expand All @@ -2112,7 +2097,7 @@ Interpreter::Value Interpreter::ProcessOpDatasetJob(CommissionerAppPtr &aCommiss
else
{
SuccessOrExit(value = aCommissioner->GetPanId(panid));
value = std::string(panid);
value = Hex(panid);
}
}
else if (CaseInsensitiveEqual(aExpr[2], "pskc"))
Expand Down Expand Up @@ -2177,7 +2162,7 @@ Interpreter::Value Interpreter::ProcessOpDatasetJob(CommissionerAppPtr &aCommiss
{
if ((dataset.mPresentFlags & ActiveOperationalDataset::kPanIdBit) != 0)
{
nwkAliases.push_back(fmt::format(FMT_STRING("0x{:04X}"), dataset.mPanId.mValue));
nwkAliases.push_back(Hex(dataset.mPanId));
}
else if ((dataset.mPresentFlags & ActiveOperationalDataset::kNetworkNameBit) != 0)
{
Expand Down Expand Up @@ -2215,10 +2200,10 @@ Interpreter::Value Interpreter::ProcessOpDatasetJob(CommissionerAppPtr &aCommiss
nwk.mName = AODS_FIELD_IF_IS_SET(NetworkName, "");
nwk.mXpan = AODS_FIELD_IF_IS_SET(ExtendedPanId, XpanId{});
nwk.mChannel = AODS_FIELD_IF_IS_SET(Channel, (Channel{0, 0})).mNumber;
nwk.mPan = AODS_FIELD_IF_IS_SET(PanId, PanId{});
nwk.mPan = AODS_FIELD_IF_IS_SET(PanId, 0);
if ((dataset.mPresentFlags & ActiveOperationalDataset::kPanIdBit) == 0)
{
nwk.mPan = PanId::kEmptyPanId;
nwk.mPan = 0;
}
else
{
Expand Down
6 changes: 4 additions & 2 deletions src/app/cli/interpreter_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,15 @@
#include "commissioner/defines.hpp"
#include "commissioner/error.hpp"
#include "commissioner/network_data.hpp"
#include "common/utils.hpp"
#include "fmt/core.h"
#include "gmock/gmock-matchers.h"
#include "gmock/gmock-spec-builders.h"
#include "gtest/gtest.h"
#include "nlohmann/json.hpp"

using namespace ot::commissioner;
using namespace ot::commissioner::utils;
using namespace ot::commissioner::persistent_storage;

using testing::_;
Expand Down Expand Up @@ -1802,7 +1804,7 @@ TEST_F(InterpreterTestSuite, PC_OpdatasetGetActive)
PersistentStorage::Status::kSuccess);
Network nwk;
EXPECT_EQ(ctx.mRegistry->mStorage->Get(nwk_id, nwk), PersistentStorage::Status::kSuccess);
EXPECT_EQ((std::string)nwk.mPan, "0x0000");
EXPECT_EQ(utils::Hex(nwk.mPan), "0x0000");

EXPECT_CALL(*ctx.mDefaultCommissionerObject, GetActiveDataset(_, _))
.Times(2)
Expand All @@ -1821,7 +1823,7 @@ TEST_F(InterpreterTestSuite, PC_OpdatasetGetActive)
EXPECT_TRUE(value.HasNoError());

EXPECT_EQ(ctx.mRegistry->mStorage->Get(nwk_id, nwk), PersistentStorage::Status::kSuccess);
EXPECT_EQ(nwk.mPan.mValue, 0x0001);
EXPECT_EQ(nwk.mPan, 0x0001);

EXPECT_EQ(system("rm -f ./aods.json"), 0);
EXPECT_NE(PathExists("./aods.json").GetCode(), ErrorCode::kNone);
Expand Down
4 changes: 2 additions & 2 deletions src/app/commissioner_app.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -680,7 +680,7 @@ Error CommissionerApp::SetNetworkName(const std::string &aNetworkName)
return error;
}

Error CommissionerApp::GetPanId(PanId &aPanId)
Error CommissionerApp::GetPanId(uint16_t &aPanId)
{
Error error;

Expand All @@ -696,7 +696,7 @@ Error CommissionerApp::GetPanId(PanId &aPanId)
return error;
}

Error CommissionerApp::SetPanId(PanId aPanId, MilliSeconds aDelay)
Error CommissionerApp::SetPanId(uint16_t aPanId, MilliSeconds aDelay)
{
Error error;
PendingOperationalDataset pendingDataset;
Expand Down
4 changes: 2 additions & 2 deletions src/app/commissioner_app.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,8 @@ class CommissionerApp : public CommissionerHandler
MOCKABLE Error SetNetworkMasterKey(const ByteArray &aMasterKey, MilliSeconds aDelay);
MOCKABLE Error GetNetworkName(std::string &aNetworkName) const;
MOCKABLE Error SetNetworkName(const std::string &aNetworkName);
MOCKABLE Error GetPanId(PanId &aPanId);
MOCKABLE Error SetPanId(PanId aPanId, MilliSeconds aDelay);
MOCKABLE Error GetPanId(uint16_t &aPanId);
MOCKABLE Error SetPanId(uint16_t aPanId, MilliSeconds aDelay);
MOCKABLE Error GetPSKc(ByteArray &aPSKc) const;
MOCKABLE Error SetPSKc(const ByteArray &aPSKc);

Expand Down
4 changes: 2 additions & 2 deletions src/app/commissioner_app_dummy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -303,13 +303,13 @@ Error CommissionerApp::SetNetworkName(const std::string &aNetworkName)
return Error{};
}

Error CommissionerApp::GetPanId(PanId &aPanId)
Error CommissionerApp::GetPanId(uint16_t &aPanId)
{
UNUSED(aPanId);
return Error{};
}

Error CommissionerApp::SetPanId(PanId aPanId, MilliSeconds aDelay)
Error CommissionerApp::SetPanId(uint16_t aPanId, MilliSeconds aDelay)
{
UNUSED(aPanId);
UNUSED(aDelay);
Expand Down
4 changes: 2 additions & 2 deletions src/app/commissioner_app_mock.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,8 @@ class CommissionerAppMock : public ::ot::commissioner::CommissionerApp
MOCK_METHOD(Error, SetNetworkMasterKey, (const ByteArray &, MilliSeconds));
MOCK_METHOD(Error, GetNetworkName, (std::string &), (const));
MOCK_METHOD(Error, SetNetworkName, (const std::string &));
MOCK_METHOD(Error, GetPanId, (PanId &));
MOCK_METHOD(Error, SetPanId, (PanId, MilliSeconds));
MOCK_METHOD(Error, GetPanId, (uint16_t &));
MOCK_METHOD(Error, SetPanId, (uint16_t, MilliSeconds));
MOCK_METHOD(Error, GetPSKc, (ByteArray &), (const));
MOCK_METHOD(Error, SetPSKc, (const ByteArray &));
MOCK_METHOD(Error, GetSecurityPolicy, (SecurityPolicy &), (const));
Expand Down
25 changes: 11 additions & 14 deletions src/app/json.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -417,18 +417,6 @@ static void from_json(const Json &aJson, SecurityPolicy &aSecurityPolicy)
#undef SET
}

static void to_json(Json &aJson, const ot::commissioner::PanId &aPanId)
{
aJson = std::string(aPanId);
}

static void from_json(const Json &aJson, ot::commissioner::PanId &aPanId)
{
std::string panIdStr;
panIdStr = aJson.get<std::string>();
SuccessOrThrow(aPanId.FromHex(panIdStr));
}

static void to_json(Json &aJson, const ActiveOperationalDataset &aDataset)
{
#define SET_IF_PRESENT(name) \
Expand All @@ -447,9 +435,13 @@ static void to_json(Json &aJson, const ActiveOperationalDataset &aDataset)
aJson["MeshLocalPrefix"] = Ipv6PrefixToString(aDataset.mMeshLocalPrefix);
};

if (aDataset.mPresentFlags & ActiveOperationalDataset::kPanIdBit)
{
aJson["PanId"] = utils::Hex(aDataset.mPanId);
}

SET_IF_PRESENT(NetworkMasterKey);
SET_IF_PRESENT(NetworkName);
SET_IF_PRESENT(PanId);
SET_IF_PRESENT(PSKc);
SET_IF_PRESENT(SecurityPolicy);

Expand Down Expand Up @@ -484,9 +476,14 @@ static void from_json(const Json &aJson, ActiveOperationalDataset &aDataset)
aDataset.mPresentFlags |= ActiveOperationalDataset::kMeshLocalPrefixBit;
};

if (aJson.contains("PanId"))
{
SuccessOrThrow(utils::ParseInteger(aDataset.mPanId, aJson["PanId"]));
aDataset.mPresentFlags |= ActiveOperationalDataset::kPanIdBit;
}

SET_IF_PRESENT(NetworkMasterKey);
SET_IF_PRESENT(NetworkName);
SET_IF_PRESENT(PanId);
SET_IF_PRESENT(PSKc);
SET_IF_PRESENT(SecurityPolicy);

Expand Down
4 changes: 2 additions & 2 deletions src/app/ps/persistent_storage_json.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ PersistentStorage::Status PersistentStorageJson::Lookup(Network const &aValue, s
(aValue.mDomainId.mId == EMPTY_ID || (el.mDomainId.mId == aValue.mDomainId.mId)) &&
(aValue.mName.empty() || (aValue.mName == el.mName)) &&
(aValue.mXpan.mValue == XpanId::kEmptyXpanId || aValue.mXpan == el.mXpan) &&
(aValue.mPan.mValue == PanId::kEmptyPanId || (aValue.mPan.mValue == el.mPan.mValue)) &&
(aValue.mPan == 0 || (aValue.mPan == el.mPan)) &&
(aValue.mMlp.empty() || CaseInsensitiveEqual(aValue.mMlp, el.mMlp)) &&
(aValue.mChannel == 0 || (aValue.mChannel == el.mChannel));

Expand Down Expand Up @@ -477,7 +477,7 @@ PersistentStorage::Status PersistentStorageJson::LookupAny(Network const &aValue
(aValue.mDomainId.mId == EMPTY_ID || (el.mDomainId.mId == aValue.mDomainId.mId)) ||
(aValue.mName.empty() || (aValue.mName == el.mName)) ||
(aValue.mXpan.mValue == XpanId::kEmptyXpanId || aValue.mXpan == el.mXpan) ||
(aValue.mPan.mValue == PanId::kEmptyPanId || (aValue.mPan.mValue == el.mPan.mValue)) ||
(aValue.mPan == 0 || (aValue.mPan == el.mPan)) ||
(aValue.mMlp.empty() || CaseInsensitiveEqual(aValue.mMlp, el.mMlp)) ||
(aValue.mChannel == 0 || (aValue.mChannel == el.mChannel));

Expand Down
26 changes: 18 additions & 8 deletions src/app/ps/registry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -484,23 +484,32 @@ Registry::Status Registry::GetNetworksByAliases(const StringArray &aAliases,
}
else
{
Network nwk;
XpanId xpid;
PanId pid;
Network nwk;
XpanId xpid;
uint16_t pid;

status = Registry::Status::kNotFound;
if (xpid.FromHex(alias) == ERROR_NONE)
{
status = GetNetworkByXpan(xpid, nwk);
}

if (status != Registry::Status::kSuccess)
{
status = GetNetworkByName(alias, nwk);
}
if (status != Registry::Status::kSuccess && pid.FromHex(alias) == ERROR_NONE)

if (status != Registry::Status::kSuccess)
{
status = GetNetworkByPan(alias, nwk);
bool hasHexPrefix = utils::ToLower(alias.substr(0, 2)) != "0x";
std::string aliasAsPanId = std::string(hasHexPrefix ? "0x" : "") + alias;

if (utils::ParseInteger(pid, aliasAsPanId) == ERROR_NONE)
{
status = GetNetworkByPan(aliasAsPanId, nwk);
}
}

if (status == Registry::Status::kSuccess)
{
networks.push_back(nwk);
Expand Down Expand Up @@ -608,9 +617,10 @@ Registry::Status Registry::GetNetworkByName(const std::string &aName, Network &a

Registry::Status Registry::GetNetworkByPan(const std::string &aPan, Network &aRet)
{
Network nwk{};
PanId panId;
if (panId.FromHex(aPan).GetCode() != ErrorCode::kNone)
Network nwk{};
uint16_t panId = 0;

if (utils::ParseInteger(panId, aPan).GetCode() != ErrorCode::kNone)
{
return Registry::Status::kError;
}
Expand Down
7 changes: 4 additions & 3 deletions src/app/ps/registry_entries.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ void to_json(json &aJson, const Network &aValue)
aJson = json{{JSON_ID, aValue.mId},
{JSON_DOM_REF, aValue.mDomainId},
{JSON_NAME, aValue.mName},
{JSON_PAN, std::string(aValue.mPan)},
{JSON_PAN, utils::Hex(aValue.mPan)},
{JSON_XPAN, std::string(aValue.mXpan)},
{JSON_CHANNEL, aValue.mChannel},
{JSON_MLP, aValue.mMlp},
Expand All @@ -165,13 +165,14 @@ void to_json(json &aJson, const Network &aValue)

void from_json(const json &aJson, Network &aValue)
{
std::string hexStr;

aJson.at(JSON_ID).get_to(aValue.mId);
aJson.at(JSON_DOM_REF).get_to(aValue.mDomainId);
aJson.at(JSON_NAME).get_to(aValue.mName);

std::string hexStr;
aJson.at(JSON_PAN).get_to(hexStr);
SuccessOrThrow(aValue.mPan.FromHex(hexStr));
SuccessOrThrow(utils::ParseInteger(aValue.mPan, hexStr));
aJson.at(JSON_XPAN).get_to(hexStr);
SuccessOrThrow(aValue.mXpan.FromHex(hexStr));

Expand Down
2 changes: 1 addition & 1 deletion src/app/ps/registry_entries.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ struct Network
std::string mName; /**< network name */
XpanId mXpan; /**< Extended PAN_ID */
unsigned int mChannel; /**< network channel */
PanId mPan; /**< PAN_ID */
uint16_t mPan; /**< PAN_ID */
std::string mMlp; /**< Mesh-local prefix */
int mCcm; /**< Commercial commissioning mode;<0 not set,
* 0 false, >0 true */
Expand Down
2 changes: 1 addition & 1 deletion src/common/error.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ namespace ot {
namespace commissioner {

// Returns the std::string representation of the status code.
static std::string ErrorCodeToString(ErrorCode code)
std::string ErrorCodeToString(ErrorCode code)
{
switch (code)
{
Expand Down
Loading

0 comments on commit ef31c91

Please sign in to comment.