Skip to content

Commit 85d0764

Browse files
coderkalyancoderkalyan
committed
rx: move transfer payload/state into reassembler
Avoid storing intermediate payload data in out_transfer, which does not need to be preserved across calls to serardRxAccept.
1 parent e41441c commit 85d0764

File tree

3 files changed

+58
-35
lines changed

3 files changed

+58
-35
lines changed

‎libserard/serard.c‎

Lines changed: 43 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -505,18 +505,15 @@ SERARD_PRIVATE bool rxTryParseHeader(const uint8_t* const payloa
505505
/// Returns <0 on error. The transfer shall be discarded and the error code returned to the user.
506506
/// Returns 0 if the header is invalid or not subscribed to. The transfer shall be silently discarded.
507507
/// Returns 1 if the header is valid and should be processed further.
508-
SERARD_PRIVATE int8_t rxValidateHeader(struct SerardRx* const ins,
509-
struct SerardReassembler* const reassembler,
510-
struct SerardRxTransfer* const out_transfer)
508+
SERARD_PRIVATE int8_t rxValidateHeader(struct SerardRx* const ins, struct SerardReassembler* const reassembler)
511509
{
512510
SERARD_ASSERT(ins != NULL);
513511
SERARD_ASSERT(reassembler != NULL);
514-
SERARD_ASSERT(out_transfer != NULL);
515512
SERARD_ASSERT(reassembler->counter == (HEADER_SIZE - 1));
516513

517514
int8_t ret = 0;
518515

519-
struct SerardTransferMetadata* const metadata = &out_transfer->metadata;
516+
struct SerardTransferMetadata* const metadata = &reassembler->metadata;
520517
SerardNodeID destination_node_id = SERARD_NODE_ID_UNSET;
521518
if (rxTryParseHeader(reassembler->header, metadata, &destination_node_id))
522519
{
@@ -547,20 +544,19 @@ SERARD_PRIVATE int8_t rxValidateHeader(struct SerardRx* const ins,
547544
// even though it is not included in the final payload size.
548545
const size_t extent = sub->extent + TRANSFER_CRC_SIZE_BYTES;
549546
SERARD_ASSERT(extent > 0);
550-
reassembler->max_payload_size = extent;
551547

552548
void* const payload = ins->memory_payload.allocate(ins->memory_payload.user_reference, extent);
553549
if (payload != NULL)
554550
{
555-
out_transfer->payload = payload;
556-
out_transfer->payload_extent = extent;
557-
ret = 1;
551+
reassembler->payload = payload;
552+
reassembler->payload_extent = extent;
553+
ret = 1;
558554
}
559555
else
560556
{
561-
out_transfer->payload = NULL;
562-
out_transfer->payload_extent = 0;
563-
ret = -SERARD_ERROR_MEMORY;
557+
reassembler->payload = NULL;
558+
reassembler->payload_extent = 0;
559+
ret = -SERARD_ERROR_MEMORY;
564560
}
565561
}
566562
else
@@ -624,8 +620,7 @@ SERARD_PRIVATE bool rxSessionUpdate(struct SerardRx* const ins,
624620
// TODO: test this
625621
SERARD_PRIVATE int8_t rxAcceptTransfer(struct SerardRx* const ins,
626622
struct SerardReassembler* const reassembler,
627-
struct SerardRxTransfer* const transfer,
628-
const uint8_t redundant_iface_index)
623+
struct SerardRxTransfer* const transfer)
629624
{
630625
SERARD_ASSERT(ins != NULL);
631626
SERARD_ASSERT(reassembler != NULL);
@@ -637,7 +632,7 @@ SERARD_PRIVATE int8_t rxAcceptTransfer(struct SerardRx* const ins,
637632
// FIXME: maybe we can just use the out_transfer->size to track the counter?
638633
const size_t payload_size = reassembler->counter - HEADER_SIZE;
639634
TransferCRC payload_crc = TRANSFER_CRC_INITIAL;
640-
payload_crc = transferCRCAdd(payload_crc, payload_size, transfer->payload);
635+
payload_crc = transferCRCAdd(payload_crc, payload_size, reassembler->payload);
641636
payload_crc = payload_crc ^ TRANSFER_CRC_OUTPUT_XOR;
642637
bool valid = payload_crc == TRANSFER_CRC_RESIDUE_AFTER_OUTPUT_XOR;
643638

@@ -714,13 +709,33 @@ SERARD_PRIVATE int8_t rxAcceptByte(struct SerardRx* const ins,
714709
{
715710
// if the state machine is accepting the payload, try to accept
716711
// the received transfer payload and return to the user
717-
ret = rxAcceptTransfer(ins, reassembler, out_transfer, redundant_iface_index);
712+
// FIXME: what is the redundant interface index for?
713+
ret = rxAcceptTransfer(ins, reassembler, out_transfer);
714+
if (ret == 1)
715+
{
716+
// copy the relevant metadata from the reassembler
717+
// because the transfer is valid, the application takes ownership
718+
// of the payload buffer.
719+
memcpy(&out_transfer->metadata, &reassembler->metadata, sizeof(struct SerardTransferMetadata));
720+
out_transfer->payload_extent = reassembler->payload_extent;
721+
out_transfer->payload = reassembler->payload;
722+
out_transfer->timestamp_usec = reassembler->timestamp_usec;
723+
}
718724
}
719725
else
720726
{
721727
// in other cases, the delimiter is premature so consider the transfer
722728
// to be invalid and silently discard it
723729
ret = 0;
730+
if (reassembler->payload != NULL)
731+
{
732+
// deallocate the payload buffer if it was allocated
733+
ins->memory_payload.deallocate(ins->memory_payload.user_reference,
734+
reassembler->payload_extent,
735+
reassembler->payload);
736+
reassembler->payload = NULL;
737+
reassembler->payload_extent = 0;
738+
}
724739
}
725740

726741
// delimiter bytes unconditionally reset the state machine
@@ -732,7 +747,7 @@ SERARD_PRIVATE int8_t rxAcceptByte(struct SerardRx* const ins,
732747
if (state_payload)
733748
{
734749
const size_t offset = reassembler->counter - HEADER_SIZE;
735-
uint8_t* const payload = out_transfer->payload;
750+
uint8_t* const payload = reassembler->payload;
736751
payload[offset] = cobs_byte;
737752
}
738753
else
@@ -744,11 +759,11 @@ SERARD_PRIVATE int8_t rxAcceptByte(struct SerardRx* const ins,
744759
{
745760
// record the fragment timestamp when the first header byte is received
746761
// see: https://github.com/OpenCyphal/pycyphal/issues/112
747-
out_transfer->timestamp_usec = timestamp_usec;
762+
reassembler->timestamp_usec = timestamp_usec;
748763
}
749764
else if (offset == (HEADER_SIZE - 1))
750765
{
751-
const int8_t out = rxValidateHeader(ins, reassembler, out_transfer);
766+
const int8_t out = rxValidateHeader(ins, reassembler);
752767
if (out < 0)
753768
{
754769
// rx pipeline encountered error, reject transfer and propogate to user
@@ -804,13 +819,15 @@ struct SerardRx serardRxInit(const struct SerardMemoryResource memory_payload,
804819
struct SerardReassembler serardReassemblerInit(void)
805820
{
806821
struct SerardReassembler reassembler = {
807-
.code = BYTE_MAX,
808-
.copy = 0,
809-
.counter = 0,
810-
.discard = false,
811-
.header = {0},
812-
.sub = NULL,
813-
.max_payload_size = 0,
822+
.counter = 0,
823+
.discard = false,
824+
.code = BYTE_MAX,
825+
.copy = 0,
826+
.metadata = {0},
827+
.header = {0},
828+
.payload_extent = 0,
829+
.payload = NULL,
830+
.sub = NULL,
814831
};
815832

816833
return reassembler;

‎libserard/serard.h‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -270,16 +270,22 @@ enum SerardInternalRxState
270270
/// Ex https://github.com/Zubax/kocherga/blob/69e2131d3a26807428f67dc2f823afd988da1bc7/kocherga/kocherga_serial.hpp#L161
271271
struct SerardReassembler
272272
{
273+
/// Reassembler state.
273274
size_t counter;
274275
bool discard;
275-
// enum SerardInternalRxState state;
276276

277+
/// COBS decoder state.
277278
uint8_t code;
278279
uint8_t copy;
279-
uint8_t header[SERARD_TRANSFER_HEADER_SIZE];
280+
281+
/// Partially decoded transfer data.
282+
struct SerardTransferMetadata metadata;
283+
uint8_t header[SERARD_TRANSFER_HEADER_SIZE]; // TODO: can we avoid storing this?
284+
size_t payload_extent;
285+
uint8_t* payload;
280286

281287
struct SerardRxSubscription* sub;
282-
size_t max_payload_size;
288+
SerardMicrosecond timestamp_usec; ///< The timestamp of the first received header byte of this transfer.
283289
};
284290

285291
/// Construct a new library instance.

‎tests/test_private.cpp‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -542,7 +542,7 @@ TEST_CASE("rxValidateHeader")
542542

543543
struct SerardRxTransfer transfer = {};
544544
const int8_t out = exposed::rxValidateHeader(&ins, &reassembler, &transfer);
545-
const auto& metadata = transfer.metadata;
545+
const auto& metadata = reassembler.metadata;
546546
REQUIRE(out == 1);
547547
REQUIRE(metadata.transfer_id == 0);
548548
REQUIRE(metadata.transfer_kind == SerardTransferKindMessage);
@@ -567,7 +567,7 @@ TEST_CASE("rxValidateHeader")
567567

568568
struct SerardRxTransfer transfer = {};
569569
const int8_t out = exposed::rxValidateHeader(&ins, &reassembler, &transfer);
570-
const auto& metadata = transfer.metadata;
570+
const auto& metadata = reassembler.metadata;
571571
REQUIRE(out == 1);
572572
REQUIRE(metadata.transfer_id == 0xCAFEB0BAUL);
573573
REQUIRE(metadata.transfer_kind == SerardTransferKindRequest);
@@ -590,7 +590,7 @@ TEST_CASE("rxValidateHeader")
590590

591591
struct SerardRxTransfer transfer = {};
592592
const int8_t out = exposed::rxValidateHeader(&ins, &reassembler, &transfer);
593-
const auto& metadata = transfer.metadata;
593+
const auto& metadata = reassembler.metadata;
594594
REQUIRE(out == 0);
595595
REQUIRE(metadata.transfer_id == 0xCAFEB0BAUL);
596596
REQUIRE(metadata.transfer_kind == SerardTransferKindRequest);
@@ -614,7 +614,7 @@ TEST_CASE("rxValidateHeader")
614614

615615
struct SerardRxTransfer transfer = {};
616616
const int8_t out = exposed::rxValidateHeader(&ins, &reassembler, &transfer);
617-
const auto& metadata = transfer.metadata;
617+
const auto& metadata = reassembler.metadata;
618618
REQUIRE(out == 0);
619619
}
620620

@@ -631,7 +631,7 @@ TEST_CASE("rxValidateHeader")
631631

632632
struct SerardRxTransfer transfer = {};
633633
const int8_t out = exposed::rxValidateHeader(&ins, &reassembler, &transfer);
634-
const auto& metadata = transfer.metadata;
634+
const auto& metadata = reassembler.metadata;
635635
REQUIRE(out == 0);
636636
}
637637

@@ -650,7 +650,7 @@ TEST_CASE("rxValidateHeader")
650650

651651
struct SerardRxTransfer transfer = {};
652652
const int8_t out = exposed::rxValidateHeader(&ins, &reassembler, &transfer);
653-
const auto& metadata = transfer.metadata;
653+
const auto& metadata = reassembler.metadata;
654654
REQUIRE(out == -SERARD_ERROR_MEMORY);
655655
}
656656
}

0 commit comments

Comments
 (0)