Fix: multiple subscription and maintain subscribed_signals coherence

across usage.

- Transmission of a reference instead of copy.
- Don't use anymore iterator on subscribed_signals map

Change-Id: I5e5b7b0bb8598be3bb0ec59c29418ee937ddcc9e
Signed-off-by: Romain Forlot <romain.forlot@iot.bzh>

diff --git a/src/can-bus.cpp b/src/can-bus.cpp
index efa6e7a..b1dcc3a 100644 (file)
--- a/src/can-bus.cpp
+++ b/src/can-bus.cpp
@@ -75,10 +75,16 @@ void can_bus_t::can_decode_message()
                {
                        {
                                std::lock_guard<std::mutex> subscribed_signals_lock(get_subscribed_signals_mutex());
-                               std::map<std::string, struct afb_event> subscribed_signals = get_subscribed_signals();
-                               const auto& it_event = subscribed_signals.find(sig.genericName);
+                               std::map<std::string, struct afb_event>& s = get_subscribed_signals();
                                
-                               if(it_event != subscribed_signals.end() && afb_event_is_valid(it_event->second))
+                               const auto& it = s.find(sig.genericName);
+                               if (it != s.end())
+                                       DEBUG(binder_interface, "Iterator key: %s, event valid? %d", it->first.c_str(), afb_event_is_valid(it->second));
+                               DEBUG(binder_interface, "Operator[] key char: %s, event valid? %d", sig.genericName, afb_event_is_valid(s[sig.genericName]));
+                               DEBUG(binder_interface, "Operator[] key string: %s, event valid? %d", sig.genericName, afb_event_is_valid(s[std::string(sig.genericName)]));
+                               DEBUG(binder_interface, "Nb elt matched char: %d", (int)s.count(sig.genericName));
+                               DEBUG(binder_interface, "Nb elt matched string: %d", (int)s.count(std::string(sig.genericName)));
+                               if( it != s.end() && afb_event_is_valid(it->second))
                                {
                                        decoded_message = decoder.translateSignal(sig, can_message, getSignals());
 
@@ -87,8 +93,8 @@ void can_bus_t::can_decode_message()
 
                                        std::lock_guard<std::mutex> decoded_can_message_lock(decoded_can_message_mutex_);
                                        push_new_vehicle_message(vehicle_message);
+                                       new_decoded_can_message_.notify_one();
                                }
-                               new_decoded_can_message_.notify_one();
                        }
                }
        }
@@ -112,13 +118,12 @@ void can_bus_t::can_event_push()
 
                {
                        std::lock_guard<std::mutex> subscribed_signals_lock(get_subscribed_signals_mutex());
-                       std::map<std::string, struct afb_event> subscribed_signals = get_subscribed_signals();
-                       const auto& it_event = subscribed_signals.find(s_message.name);
-                       if(it_event != subscribed_signals.end() && afb_event_is_valid(it_event->second))
+                       std::map<std::string, struct afb_event>& s = get_subscribed_signals();
+                       if(s.find(std::string(s_message.name)) != s.end() && afb_event_is_valid(s[std::string(s_message.name)]))
                        {
                                jo = json_object_new_object();
                                jsonify_simple(s_message, jo);
-                               afb_event_push(it_event->second, jo);
+                               afb_event_push(s[std::string(s_message.name)], jo);
                        }
                }
        }

diff --git a/src/can-signals.cpp b/src/can-signals.cpp
index 82ee0de..3832c33 100644 (file)
--- a/src/can-signals.cpp
+++ b/src/can-signals.cpp
@@ -34,6 +34,16 @@ std::vector<std::vector<CanSignal>> SIGNALS = {
        {{&(CAN_MESSAGES[0][0]), "can.driver_door.open", 2, 4, 1.000000, 0.000000, 0.000000, 0.000000, {10, 0, nullptr}, false, true, nullptr, 0, false, decoder_t::booleanDecoder, nullptr, false, (float)NULL}},
 };
 
+/** 
+ * @brief Can signal event map making access to afb_event
+ * externaly to an openxc existing structure.
+ *
+ * @desc Event map is making relation between CanSignal generic name
+ * and the afb_event struct used by application framework to pushed
+ * to the subscriber.
+ */
+std::map<std::string, struct afb_event> subscribed_signals;
+
 /**
 * @brief Mutex allowing safe manipulation on subscribed_signals map.
 * @desc To ensure that the map object isn't modified when we read it, you
@@ -46,7 +56,13 @@ std::mutex& get_subscribed_signals_mutex()
        return subscribed_signals_mutex;
 }
 
-const std::vector<CanSignal> getSignals()
+std::map<std::string, struct afb_event>& get_subscribed_signals()
+{
+       DEBUG(binder_interface, "Here are the first subscribed_signals: %s", subscribed_signals.begin()->first.c_str() );
+       return subscribed_signals;
+}
+
+const std::vector<CanSignal>& getSignals()
 {
        return SIGNALS[MESSAGE_SET_ID];
 }
@@ -88,9 +104,4 @@ std::vector<CanSignal> find_can_signals(const openxc_DynamicField &key)
 inline uint32_t get_CanSignal_id(const CanSignal& sig)
 {
        return sig.message->id;
-}
-
-const std::map<std::string, struct afb_event> get_subscribed_signals()
-{
-       return subscribed_signals;
 }
\ No newline at end of file

diff --git a/src/can-signals.hpp b/src/can-signals.hpp
index a6248f2..a95b3a3 100644 (file)
--- a/src/can-signals.hpp
+++ b/src/can-signals.hpp
@@ -36,18 +36,17 @@ extern "C"
 
 #define MESSAGE_SET_ID 0
 
-/** 
- * @brief Can signal event map making access to afb_event
- * externaly to an openxc existing structure.
- *
- * @desc Event map is making relation between CanSignal generic name
- * and the afb_event struct used by application framework to pushed
- * to the subscriber.
- */
-static std::map<std::string, struct afb_event> subscribed_signals;
-
+extern std::mutex subscribed_signals_mutex;
 std::mutex& get_subscribed_signals_mutex();
 
+/**
+ * @brief return the subscribed_signals map.
+ * 
+ * return std::map<std::string, struct afb_event> - map of subscribed signals.
+ */
+extern std::map<std::string, struct afb_event> subscribed_signals;
+std::map<std::string, struct afb_event>& get_subscribed_signals();
+
 /**
  * @brief The type signature for a CAN signal decoder.
  *
@@ -135,7 +134,7 @@ typedef struct CanSignal CanSignal;
 
 /* Public: Return signals from an signals array filtered on name.
  */
-const std::vector<CanSignal> getSignals();
+const std::vector<CanSignal>& getSignals();
 
 /* Public: Return the length of the array returned by getSignals(). */
 size_t getSignalCount();
@@ -158,11 +157,4 @@ std::vector<CanSignal> find_can_signals(const openxc_DynamicField &key);
  *
  * @return uint32_t - unsigned integer representing the arbitration id.
  */
-inline uint32_t get_CanSignal_id(const CanSignal& sig);
-
-/**
- * @brief return the subscribed_signals map.
- * 
- * return std::map<std::string, struct afb_event> - map of subscribed signals.
- */
-const std::map<std::string, struct afb_event> get_subscribed_signals();
\ No newline at end of file
+inline uint32_t get_CanSignal_id(const CanSignal& sig);
\ No newline at end of file

diff --git a/src/low-can-binding.cpp b/src/low-can-binding.cpp
index c5f10f9..420ad96 100644 (file)
--- a/src/low-can-binding.cpp
+++ b/src/low-can-binding.cpp
@@ -52,10 +52,10 @@ can_bus_t *can_bus_handler;
 *
 *********************************************************************************/
 
-static int make_subscription_unsubscription(struct afb_req request, const char* sig_name, bool subscribe)
+static int make_subscription_unsubscription(struct afb_req request, const char* sig_name, std::map<std::string, struct afb_event>& s, bool subscribe)
 {
        /* Make the subscription or unsubscription to the event */
-       if (((subscribe ? afb_req_subscribe : afb_req_unsubscribe)(request, subscribed_signals[std::string(sig_name)])) < 0)
+       if (((subscribe ? afb_req_subscribe : afb_req_unsubscribe)(request, s[std::string(sig_name)])) < 0)
        {
                ERROR(binder_interface, "Operation goes wrong for signal: %s", sig_name);
                return 0;
@@ -64,10 +64,10 @@ static int make_subscription_unsubscription(struct afb_req request, const char*
 
 }
 
-static int create_event_handle(const char* sig_name)
+static int create_event_handle(const char* sig_name, std::map<std::string, struct afb_event>& s)
 {
-       subscribed_signals[std::string(sig_name)] = afb_daemon_make_event(binder_interface->daemon, sig_name);
-       if (!afb_event_is_valid(subscribed_signals[std::string(sig_name)]))
+       s[std::string(sig_name)] = afb_daemon_make_event(binder_interface->daemon, sig_name);
+       if (!afb_event_is_valid(s[std::string(sig_name)]))
        {
                ERROR(binder_interface, "Can't create an event, something goes wrong.");
                return 0;
@@ -80,7 +80,8 @@ static int subscribe_unsubscribe_signal(struct afb_req request, bool subscribe,
        int ret;
 
        std::lock_guard<std::mutex> subscribed_signals_lock(get_subscribed_signals_mutex());
-       if (subscribed_signals.find(sig.genericName) != subscribed_signals.end() && !afb_event_is_valid(subscribed_signals[std::string(sig.genericName)]))
+       std::map<std::string, struct afb_event>& s = get_subscribed_signals();
+       if (s.find(sig.genericName) != s.end() && !afb_event_is_valid(s[std::string(sig.genericName)]))
        {
                if(!subscribe)
                {
@@ -89,14 +90,14 @@ static int subscribe_unsubscribe_signal(struct afb_req request, bool subscribe,
                }
                else
                        /* Event it isn't valid annymore, recreate it */
-                       ret = create_event_handle(sig.genericName);
+                       ret = create_event_handle(sig.genericName, s);
        }
        else
        {
                /* Event don't exist , so let's create it */
                struct afb_event empty_event = {nullptr, nullptr};
                subscribed_signals[sig.genericName] = empty_event;
-               ret = create_event_handle(sig.genericName);
+               ret = create_event_handle(sig.genericName, s);
        }
 
        /* Check whether or not the event handler has been correctly created and
@@ -104,22 +105,35 @@ static int subscribe_unsubscribe_signal(struct afb_req request, bool subscribe,
         */
        if (ret <= 0)
                return ret;
-       return make_subscription_unsubscription(request, sig.genericName, subscribe);
+       return make_subscription_unsubscription(request, sig.genericName, s, subscribe);
 }
 
+/**
+ * @fn static int subscribe_unsubscribe_signals(struct afb_req request, bool subscribe, const std::vector<CanSignal>& signals)
+ * @brief subscribe to all signals in the vector signals
+ *
+ * @param[in] afb_req request : contain original request use to subscribe or unsubscribe
+ * @param[in] subscribe boolean value used to chose between a subscription operation or an unsubscription
+ * @param[in] CanSignal  vector with CanSignal to subscribe
+ *
+ * @return Number of correctly subscribed signal
+ */
 static int subscribe_unsubscribe_signals(struct afb_req request, bool subscribe, const std::vector<CanSignal>& signals)
 {
-       int ret = 0;
+       int rets = 0;
 
        for(const auto& signal_i : signals)
        {
-               ret = subscribe_unsubscribe_signal(request, subscribe, signal_i);
-               if(ret == 0)
+               int ret = subscribe_unsubscribe_signal(request, subscribe, signal_i);
+               if(ret <= 0)
                        return ret;
+               rets++;
+               DEBUG(binder_interface, "Signal: %s subscribed", signal_i.genericName);
        }
-       return ret;
+       return rets;
 }
 
+// TODO
 static int subscribe_unsubscribe_all(struct afb_req request, bool subscribe)
 {
        int e = 0;
@@ -153,6 +167,7 @@ static int subscribe_unsubscribe_name(struct afb_req request, bool subscribe, co
                                ret = 0;
                }
                ret = subscribe_unsubscribe_signals(request, subscribe, sig);
+               NOTICE(binder_interface, "Subscribed correctly to %d/%d signal(s).", ret, (int)sig.size());
        }
        return ret;
 }