Skip to content

PrmDb: use canonical serialized size for record size #4954

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hidirektor wants to merge 4 commits into
base: devel
Choose a base branch
from
+133 −15
Open

PrmDb: use canonical serialized size for record size #4954

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3bcfe03
PrmDb: use canonical serialized size for record size and add 16-bit r…
hidirektor Apr 4, 2026
80b2620
PrmDb: format PrmDbImpl with fprime-util
hidirektor Apr 6, 2026
4155757
Merge branch 'devel' into devel
hidirektor Apr 6, 2026
109e5ef
Merge branch 'nasa:devel' into devel
hidirektor Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions Svc/PrmDb/PrmDbImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,12 +200,12 @@ void PrmDbImpl::PRM_SAVE_FILE_cmdHandler(FwOpcodeType opCode, U32 cmdSeq) {
// add delimiter to CRC
crc = this->computeCrc(crc, &delim, sizeof(delim));

// serialize record size = id field + data
U32 recordSize = static_cast<U32>(sizeof(FwPrmIdType) + entry.getValue().getSize());
// serialize record size = id field + data using the canonical F Prime size format
FwSizeType recordSize = static_cast<FwSizeType>(sizeof(FwPrmIdType) + entry.getValue().getSize());

// reset buffer
buff.resetSer();
Fw::SerializeStatus serStat = buff.serializeFrom(recordSize);
Fw::SerializeStatus serStat = buff.serializeSize(recordSize);
// should always work
FW_ASSERT(Fw::FW_SERIALIZE_OK == serStat, static_cast<FwAssertArgType>(serStat));

Expand All @@ -218,7 +218,7 @@ void PrmDbImpl::PRM_SAVE_FILE_cmdHandler(FwOpcodeType opCode, U32 cmdSeq) {
this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR);
return;
}
if (writeSize != sizeof(recordSize)) {
if (writeSize != static_cast<FwSizeType>(sizeof(FwSizeStoreType))) {
this->unLock();
this->log_WARNING_HI_PrmFileWriteError(PrmWriteError::RECORD_SIZE_SIZE, static_cast<I32>(numRecords),
static_cast<I32>(writeSize));
Expand Down Expand Up @@ -477,17 +477,17 @@ PrmDbImpl::PrmLoadStatus PrmDbImpl::readParamFileImpl(const Fw::StringBase& file
return PrmLoadStatus::ERROR;
}

U32 recordSize = 0;
FwSizeType recordSize = 0;

// read record size
readSize = sizeof(recordSize);
// read canonical record size field
readSize = static_cast<FwSizeType>(sizeof(FwSizeStoreType));

fStat = paramFile.read(buff.getBuffAddr(), readSize, Os::File::WaitType::WAIT);
if (fStat != Os::File::OP_OK) {
this->log_WARNING_HI_PrmFileReadError(PrmReadError::RECORD_SIZE, static_cast<I32>(recordNumTotal), fStat);
return PrmLoadStatus::ERROR;
}
if (sizeof(recordSize) != readSize) {
if (static_cast<FwSizeType>(sizeof(FwSizeStoreType)) != readSize) {
this->log_WARNING_HI_PrmFileReadError(PrmReadError::RECORD_SIZE_SIZE, static_cast<I32>(recordNumTotal),
static_cast<I32>(readSize));
return PrmLoadStatus::ERROR;
Expand All @@ -499,12 +499,13 @@ PrmDbImpl::PrmLoadStatus PrmDbImpl::readParamFileImpl(const Fw::StringBase& file
// reset deserialization
buff.resetDeser();
// deserialize, since record size is serialized in file
desStat = buff.deserializeTo(recordSize);
desStat = buff.deserializeSize(recordSize);
FW_ASSERT(Fw::FW_SERIALIZE_OK == desStat);

// sanity check value. It can't be larger than the maximum parameter buffer size + id
// or smaller than the record id
if ((recordSize > FW_PARAM_BUFFER_MAX_SIZE + sizeof(U32)) or (recordSize < sizeof(U32))) {
if ((recordSize > FW_PARAM_BUFFER_MAX_SIZE + sizeof(FwPrmIdType)) or
(recordSize < static_cast<FwSizeType>(sizeof(FwPrmIdType)))) {
this->log_WARNING_HI_PrmFileReadError(PrmReadError::RECORD_SIZE_VALUE, static_cast<I32>(recordNumTotal),
static_cast<I32>(recordSize));
return PrmLoadStatus::ERROR;
Expand Down
21 changes: 21 additions & 0 deletions Svc/PrmDb/test/ut/PrmDbTestMain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,27 @@ TEST(ParameterDbTest, PrmFileLoadIllegalActions) {
tester.runPrmFileLoadIllegal();
}

#if FW_HAS_16_BIT == 1
TEST(ParameterDbTest, PrmFileLoad16BitRoundTrip) {
TEST_CASE(105.1.4, "16-bit file load/save round trip");
COMMENT("Verify parameter database file format round-trips a 16-bit parameter value and canonical size field.");

Svc::PrmDbImpl impl("PrmDbImpl");

impl.init(10, 0);
impl.configure("TestFile.prm");

Svc::PrmDbTester tester(impl);

tester.init();

// connect ports
connectPorts(impl, tester);

tester.runPrmFileLoad16BitRoundTrip();
}
#endif

int main(int argc, char** argv) {
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
Expand Down
104 changes: 98 additions & 6 deletions Svc/PrmDb/test/ut/PrmDbTester.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <Fw/Com/ComBuffer.hpp>
#include <Fw/Com/ComPacket.hpp>
#include <Fw/Test/UnitTest.hpp>
#include <Fw/Types/Serializable.hpp>
#include <Os/Delegate.hpp>
#include <Os/Stub/test/File.hpp>
#include <Svc/PrmDb/test/ut/PrmDbTester.hpp>
Expand Down Expand Up @@ -139,6 +140,97 @@ void PrmDbTester::runNominalSaveFile() {
ASSERT_EVENTS_PrmFileSaveComplete(0, 2);
}

#if FW_HAS_16_BIT == 1
void PrmDbTester::runPrmFileLoad16BitRoundTrip() {
// Populate the database with a 16-bit payload to exercise the size-field serialization path.
this->m_impl.clearDb(PrmDbType::DB_ACTIVE);
this->m_impl.clearDb(PrmDbType::DB_STAGING);

Os::Stub::File::Test::StaticData::setWriteResult(m_io_data, sizeof m_io_data);
Os::Stub::File::Test::StaticData::setNextStatus(Os::File::OP_OK);

const U16 value = static_cast<U16>(0x1234);
const FwPrmIdType id = static_cast<FwPrmIdType>(0x42);

Fw::ParamBuffer pBuff;
Fw::SerializeStatus stat = pBuff.serializeFrom(value);
EXPECT_EQ(Fw::FW_SERIALIZE_OK, stat);

this->clearEvents();
this->clearHistory();
this->invoke_to_setPrm(0, id, pBuff);
Fw::QueuedComponentBase::MsgDispatchStatus dispatchStatus = this->m_impl.doDispatch();
EXPECT_EQ(dispatchStatus, Fw::QueuedComponentBase::MSG_DISPATCH_OK);

ASSERT_EVENTS_SIZE(1);
ASSERT_EVENTS_PrmIdAdded_SIZE(1);
ASSERT_EVENTS_PrmIdAdded(0, id);

// Save the database to disk so we can validate the serialized file layout.
Os::Stub::File::Test::StaticData::setWriteResult(m_io_data, sizeof m_io_data);
Os::Stub::File::Test::StaticData::setNextStatus(Os::File::OP_OK);

this->clearEvents();
this->clearHistory();
this->sendCmd_PRM_SAVE_FILE(0, 12);
dispatchStatus = this->m_impl.doDispatch();
EXPECT_EQ(dispatchStatus, Fw::QueuedComponentBase::MSG_DISPATCH_OK);

ASSERT_CMD_RESPONSE_SIZE(1);
ASSERT_CMD_RESPONSE(0, PrmDbImpl::OPCODE_PRM_SAVE_FILE, 12, Fw::CmdResponse::OK);
ASSERT_EVENTS_SIZE(1);
ASSERT_EVENTS_PrmFileSaveComplete_SIZE(1);
ASSERT_EVENTS_PrmFileSaveComplete(0, 1);

// Verify the on-disk layout is canonical and can be parsed independently of the host type sizes.
Fw::ExternalSerializeBuffer fileBuff(m_io_data,
static_cast<Fw::Serializable::SizeType>(Os::Stub::File::Test::StaticData::data.pointer));
fileBuff.resetDeser();

U32 crc = 0;
stat = fileBuff.deserializeTo(crc);
EXPECT_EQ(Fw::FW_SERIALIZE_OK, stat);

U8 delim = 0;
stat = fileBuff.deserializeTo(delim);
EXPECT_EQ(Fw::FW_SERIALIZE_OK, stat);
EXPECT_EQ(PRMDB_ENTRY_DELIMITER, delim);

FwSizeType recordSize = 0;
stat = fileBuff.deserializeSize(recordSize);
EXPECT_EQ(Fw::FW_SERIALIZE_OK, stat);
EXPECT_EQ(recordSize, static_cast<FwSizeType>(sizeof(FwPrmIdType) + sizeof(U16)));

FwPrmIdType decodedId = 0;
stat = fileBuff.deserializeTo(decodedId);
EXPECT_EQ(Fw::FW_SERIALIZE_OK, stat);
EXPECT_EQ(id, decodedId);

U16 decodedValue = 0;
stat = fileBuff.deserializeTo(decodedValue);
EXPECT_EQ(Fw::FW_SERIALIZE_OK, stat);
EXPECT_EQ(value, decodedValue);

// Re-load the saved file and verify that the 16-bit parameter round-trips successfully.
Os::Stub::File::Test::StaticData::setReadResult(m_io_data, Os::Stub::File::Test::StaticData::data.pointer);
Os::Stub::File::Test::StaticData::setNextStatus(Os::File::OP_OK);

this->clearEvents();
this->m_impl.readParamFile();

ASSERT_EVENTS_SIZE(1);
ASSERT_EVENTS_PrmFileLoadComplete_SIZE(1);
ASSERT_EVENTS_PrmFileLoadComplete(0, "ACTIVE", 1, 1, 0);

pBuff.resetSer();
this->invoke_to_getPrm(0, id, pBuff);
U16 verifyValue = 0;
stat = pBuff.deserializeTo(verifyValue);
EXPECT_EQ(Fw::FW_SERIALIZE_OK, stat);
EXPECT_EQ(value, verifyValue);
}
#endif

void PrmDbTester::runNominalLoadFile() {
// Preconditions to populate the write file
this->runNominalSaveFile();
Expand Down Expand Up @@ -371,11 +463,11 @@ void PrmDbTester::runFileReadError() {
break;
case 2:
ASSERT_EVENTS_PrmFileReadError_SIZE(1);
ASSERT_EVENTS_PrmFileReadError(0, PrmReadError::RECORD_SIZE_SIZE, 0, sizeof(U32) + 1);
ASSERT_EVENTS_PrmFileReadError(0, PrmReadError::RECORD_SIZE_SIZE, 0, sizeof(FwSizeStoreType) + 1);
break;
case 3:
ASSERT_EVENTS_PrmFileReadError_SIZE(1);
ASSERT_EVENTS_PrmFileReadError(0, PrmReadError::PARAMETER_ID_SIZE, 0, sizeof(FwPrmIdType) + 1);
ASSERT_EVENTS_PrmFileReadError(0, PrmReadError::RECORD_SIZE_SIZE, 0, sizeof(FwSizeStoreType) + 1);
Copy link
Copy Markdown
Collaborator

@LeStarch LeStarch Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change? I believe this loops through each record. Size was in case 2, so my expectation that case 3 remain PARAMETER_ID

Copy link
Copy Markdown
Author

@hidirektor hidirektor Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may seem like an odd change at first, but this test fails by read() call index (m_waits), not by field name. With the size-format update, case 3 now hits a record-size read at the next record boundary, so PrmReadError::RECORD_SIZE_SIZE is the correct expectation

break;
case 4:
ASSERT_EVENTS_PrmFileReadError_SIZE(1);
Expand Down Expand Up @@ -460,9 +552,9 @@ void PrmDbTester::runFileReadError() {
break;
case 2: {
// Data in this test is corrupted by adding 1 to the first data byte read. Since data is stored in
// big-endian format the highest order byte of the record size (U32) must have one added to it.
// Expected result of '8' inherited from original design of test.
U32 expected_error_value = sizeof(FwPrmIdType) + 4 + (1 << ((sizeof(U32) - 1) * 8));
// big-endian format the highest order byte of the record size field must have one added to it.
FwSizeType expected_error_value = static_cast<FwSizeType>(sizeof(FwPrmIdType) + 4) +
(static_cast<FwSizeType>(1) << ((sizeof(FwSizeStoreType) - 1) * 8));
ASSERT_EVENTS_PrmFileReadError_SIZE(1);
ASSERT_EVENTS_PrmFileReadError(0, PrmReadError::RECORD_SIZE_VALUE, 0, expected_error_value);
break;
Expand Down Expand Up @@ -516,7 +608,7 @@ void PrmDbTester::runFileWriteError() {
break;
case 1:
ASSERT_EVENTS_PrmFileWriteError_SIZE(1);
ASSERT_EVENTS_PrmFileWriteError(0, PrmWriteError::RECORD_SIZE_SIZE, 0, sizeof(U32) + 1);
ASSERT_EVENTS_PrmFileWriteError(0, PrmWriteError::RECORD_SIZE_SIZE, 0, sizeof(FwSizeStoreType) + 1);
break;
case 2:
ASSERT_EVENTS_PrmFileWriteError_SIZE(1);
Expand Down
3 changes: 3 additions & 0 deletions Svc/PrmDb/test/ut/PrmDbTester.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ class PrmDbTester : public PrmDbGTestBase {
void runPrmFileLoadNominal();
void runPrmFileLoadWithErrors();
void runPrmFileLoadIllegal();
#if FW_HAS_16_BIT == 1
void runPrmFileLoad16BitRoundTrip();
#endif

void runRefPrmFile();

Expand Down
Loading