better coalescence

Author: pavel-kirienko

libcanard/canard.c

@@ -1640,22 +1640,37 @@ static canard_filter_t rx_filter_fuse(const canard_filter_t a, const canard_filt
 // Filter selectivity metric; see the Cyphal/CAN specification. Greater values ==> stronger filter.
 static byte_t rx_filter_rank(const canard_filter_t a) { return popcount(a.extended_mask); }
 
-// Modifies the filter array such that the new filter is accepted. See Cyphal/CAN Spec.
+// Modifies the filter array such that the new filter is accepted. See Cyphal/CAN Spec. Requires count>0.
 static void rx_filter_coalesce_into(const size_t count, canard_filter_t* const into, const canard_filter_t new)
 {
-    // Currently we're using a simplified fast algorithm that doesn't attempt fusing existing entries with each other.
-    // This may result in suboptimal configurations but is faster.
-    size_t index     = 0;
-    byte_t best_rank = 0;
+    CANARD_ASSERT((count > 0U) && (into != NULL));
+    // Find the most similar pair by trying all pair combinations (including the incoming filter).
+    // This is O(N^2) but yields better final filter quality than fusing only with the incoming filter.
+    // The complexity is acceptable because N is controlled by the application and is small;
+    // plus, this behavior can be disabled completely by using at least as many filters as there are subscriptions.
+    bool            initialized = false;
+    size_t          best_i      = 0;
+    size_t          best_j      = count;
+    byte_t          best_rank   = 0;
+    canard_filter_t best_fuse   = { 0 };
     for (size_t i = 0; i < count; i++) {
-        const byte_t fused_rank = rx_filter_rank(rx_filter_fuse(into[i], new));
-        if (fused_rank >= best_rank) { // use >= to prefer later filters where v0 is
-            best_rank = fused_rank;
-            index     = i;
+        for (size_t j = i + 1U; j <= count; j++) { // j==count denotes the incoming filter
+            const canard_filter_t f = rx_filter_fuse(into[i], (j < count) ? into[j] : new);
+            const byte_t          r = rx_filter_rank(f);
+            if ((!initialized) || (r >= best_rank)) { // use >= to prefer later filters where v0 is
+                initialized = true;
+                best_i      = i;
+                best_j      = j;
+                best_rank   = r;
+                best_fuse   = f;
+            }
         }
     }
-    CANARD_ASSERT(index < count);
-    into[index] = rx_filter_fuse(into[index], new);
+    CANARD_ASSERT(initialized && (best_i < count) && (best_j <= count));
+    into[best_i] = best_fuse;
+    if (best_j < count) {
+        into[best_j] = new;
+    }
 }
 
 // Recompute the filter configuration and apply. Returns true on success, false on driver error.

libcanard/canard.h

@@ -386,12 +386,12 @@ struct canard_t
 /// and collisions, and will automatically migrate to a free node-ID shall a collision be detected.
 /// If manual allocation is desired, use the corresponding function to set the node-ID after initialization.
 ///
-/// The filter count is the number of CAN acceptance filters that the library can utilize. If there are fewer filters
-/// than subscriptions, similar filters will be coalesced. It is possible to pass zero filters if filtering is
-/// unneeded/unsupported. When the number of active subscriptions exceeds the number of available filters,
-/// filter coalescence is performed, which however has a high complexity bound; it is thus recommended that the number
-/// of filters is either large enough to accommodate all subscriptions, or small enough in the single digits where
-/// the load remains low.
+/// The filter count is the number of CAN acceptance filters that the stack can utilize. It is possible to pass zero
+/// filters if filtering is unneeded/unsupported. When the number of active subscriptions exceeds the number of
+/// available filters, filter coalescence is performed, which however has a high complexity bound; it is thus
+/// recommended that the number of filters is either large enough to accommodate all subscriptions,
+/// or small enough in the single digits where the coalescence load remains low. The filter configuration is
+/// recomputed and applied on every poll() following a change in the subscription set or local node-ID.
 ///
 /// CAN FD mode is selected by default for outgoing frames; override the fd flag to change the mode if needed.
 ///

tests/src/test_intrusive_rx_filter.c

@@ -236,6 +236,21 @@ static void test_rx_filter_coalesce_into_tie_prefers_later_index(void)
     TEST_ASSERT_EQUAL_HEX32(fused_index1.extended_mask, into[1].extended_mask);
 }
 
+static void test_rx_filter_coalesce_into_merges_existing_pair_when_best(void)
+{
+    canard_filter_t into[2] = {
+        { .extended_can_id = 0x0UL, .extended_mask = 0xFUL },
+        { .extended_can_id = 0x1UL, .extended_mask = 0xFUL },
+    };
+    const canard_filter_t new_filter     = { .extended_can_id = 0xFUL, .extended_mask = 0xFUL };
+    const canard_filter_t fused_existing = rx_filter_fuse(into[0], into[1]);
+    rx_filter_coalesce_into(2U, into, new_filter);
+    TEST_ASSERT_EQUAL_HEX32(fused_existing.extended_can_id, into[0].extended_can_id);
+    TEST_ASSERT_EQUAL_HEX32(fused_existing.extended_mask, into[0].extended_mask);
+    TEST_ASSERT_EQUAL_HEX32(new_filter.extended_can_id, into[1].extended_can_id);
+    TEST_ASSERT_EQUAL_HEX32(new_filter.extended_mask, into[1].extended_mask);
+}
+
 static void test_rx_filter_coalesce_into_single_entry(void)
 {
     canard_filter_t       into[1]       = { { .extended_can_id = 0x3UL, .extended_mask = 0xFUL } };
@@ -268,6 +283,7 @@ int main(void)
 
     RUN_TEST(test_rx_filter_coalesce_into_selects_best_rank);
     RUN_TEST(test_rx_filter_coalesce_into_tie_prefers_later_index);
+    RUN_TEST(test_rx_filter_coalesce_into_merges_existing_pair_when_best);
     RUN_TEST(test_rx_filter_coalesce_into_single_entry);
 
     return UNITY_END();