From 2cc61da15f8de6f5062df5d683aa13b6cdbe1264 Mon Sep 17 00:00:00 2001
From: Olli Huopio <olli.huopio@atostek.com>
Date: Thu, 14 Mar 2024 11:28:02 +0200
Subject: [PATCH] Adjust GAP messages so FastDDS always accepts them, while
 still complying with the spec.

Always set gapList.base as the exclusive endpoint of the contiguous range of sequence numbers in the GAP message. Before, we set gapList.base as the inclusive endpoint. This was not against the spec, but it meant that we could send messages where gap_start == gapList.base, in which case some DDS implementations (at least FastDDS) refused to process the GAP message. The new implementation also complies with the spec.
---
 src/messages/submessages/gap.rs | 18 +++++++++++-------
 src/rtps/message.rs             | 20 +++++++++-----------
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/src/messages/submessages/gap.rs b/src/messages/submessages/gap.rs
index 5ce69ecc..b13806e2 100644
--- a/src/messages/submessages/gap.rs
+++ b/src/messages/submessages/gap.rs
@@ -16,9 +16,9 @@ use super::{
   submessage_kind::SubmessageKind,
 };
 /// This Submessage is sent from an RTPS Writer to an RTPS Reader and
-/// indicates to the RTPS Reader that a range of sequence numbers
-/// is no longer relevant. The set may be a contiguous range of
-/// sequence numbers or a specific set of sequence numbers.
+/// indicates to the RTPS Reader that a set of sequence numbers
+/// is no longer relevant. The set may contain a contiguous range of sequence
+/// numbers and/or a noncontiguous collection of sequence numbers.
 #[derive(Debug, PartialEq, Eq, Clone, Readable, Writable)]
 pub struct Gap {
   /// Identifies the Reader Entity that is being informed of the
@@ -29,15 +29,19 @@ pub struct Gap {
   /// numbers applies.
   pub writer_id: EntityId,
 
-  /// Identifies the first sequence number in the interval of
+  /// Identifies the first sequence number in the contiguous range of
   /// irrelevant sequence numbers
   pub gap_start: SequenceNumber,
 
-  /// Identifies the last sequence number in the interval of irrelevant
-  /// sequence numbers.
+  /// Identifies the last sequence number in the contiguous range of irrelevant
+  /// sequence numbers as defined by the spec: gap_list.base is the exclusive
+  /// endpoint of this range. Therefore, the range contains the sequence
+  /// numbers which satisfy gap_start <= sn < gap_list.base.
   ///
   /// Identifies an additional list of sequence numbers that are
-  /// irrelevant.
+  /// irrelevant. Note that gap_list.base may or may not be included in this
+  /// list (and if it is, then the contiguous range of irrelevant
+  /// sequence numbers is actually larger than gap_start <= sn < gap_list.base).
   pub gap_list: SequenceNumberSet,
 }
 
diff --git a/src/rtps/message.rs b/src/rtps/message.rs
index ed9406f2..1b41e1a6 100644
--- a/src/rtps/message.rs
+++ b/src/rtps/message.rs
@@ -447,18 +447,16 @@ impl MessageBuilder {
   ) -> Self {
     match (irrelevant_sns.first(), irrelevant_sns.last()) {
       (Some(&gap_start), Some(&_last_sn)) => {
-        // Determine the contiguous range (starting from gap_start) of irrelevant
-        // seqnums
-        let mut range_end = gap_start;
-        while irrelevant_sns.contains(&range_end.plus_1()) {
-          range_end = range_end.plus_1();
+        // Determine the contiguous range of irrelevant seqnums starting from gap_start.
+        // Do this by finding the first seqnum which is larger than gap_start but is not
+        // included in irrelevant_sns. That is, find the
+        // exclusive endpoint of the contiguous range.
+        let mut range_endpoint_excl = gap_start.plus_1();
+        while irrelevant_sns.contains(&range_endpoint_excl) {
+          range_endpoint_excl = range_endpoint_excl.plus_1();
         }
-        // range_end is now the last seqnum in the contiguous range.
-        // Note that when receiving a GAP, the interpreted range is
-        // (gap_start .. gapList.base -1). But since gapList.base is included in the
-        // gapList, gapList.base is also always interpred as irrelevant, hence
-        // completing the range
-        let list_base = range_end;
+        // Set gapList.base as this exclusive endpoint of the range
+        let list_base = range_endpoint_excl;
         let list_set = irrelevant_sns.clone().split_off(&list_base);
         let gap_list = SequenceNumberSet::from_base_and_set(list_base, &list_set);