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>

4 files changed, 69 insertions, 46 deletions

diff --git a/src/can-bus.cpp b/src/can-bus.cpp
index efa6e7ac..b1dcc3ad 100644
--- 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 82ee0dea..3832c331 100644
--- 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 a6248f29..a95b3a3a 100644
--- a/src/can-signals.hpp
+++ b/src/can-signals.hpp
@@ -36,19 +36,18 @@ 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.
  *
  * @desc A SignalDecoder transforms a raw floating point CAN signal into a number,
@@ -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 c5f10f9b..420ad961 100644
--- 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;
 }