Michael, You're right, that's the best approach ; thanks for sharing it with me. Here is a PR : https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/777 for review. (I also converted the second parameter - metric name - but you can remove this commit if you prefer) Regards, Le 18/11/2022 à 14:51, Michael Balzer a écrit :
Ludovic,
there is an even better variant of your second approach, that's changing MetricCallbackEntry.m_caller to a std::string.
That's the same scheme EventCallbackEntry already uses, and m_caller is only used to identiy the registrant, so that's straight forward.
Regards, Michael
Am 18.11.22 um 01:49 schrieb Ludovic LANGE:
Some news...
My issue was caused because of course I made a mistake with the way of registering my metrics Listener.
I used the following code (see my previous post) in the constructor:
+ MyMetrics.RegisterListener(IDTAG.c_str(), "*", std::bind(&canlog::MetricListener, this, _1));
and for the destructor: + MyMetrics.DeregisterListener(IDTAG.c_str());
I had to use IDTAG because this class has multiple instances.
The issue is passing IDTAG.c_str() as a parameter ; becauseMyMetrics.RegisterListener stores the value of this const char* parameter and uses it 1) as an immutable parameter and 2) as a value.
Of course this does not work with my first way to call MyMetrics.RegisterListener :
* the memory buffer returned by IDTAG.c_str() has no more existence after exiting the constructor, so this lead to memory corruption. * when calling IDTAG.c_str() again in the destructor, there is absolutely no chance that this c_str() call returns the same memory address as the call in the constructor, so the value comparison will never work.
I rewrote the code this way by adding a new member (m_idtag) to the class:
@ vehicle/OVMS.V3/components/can/src/canlog.cpp:352 @ canlog::canlog(const char* type, std::string format, canformat::canformat_serve_ m_dropcount = 0; m_filtercount = 0;
+ m_idtag = strdup(IDTAG.c_str()); + using std::placeholders::_1; using std::placeholders::_2; MyEvents.RegisterEvent(IDTAG, "*", std::bind(&canlog::EventListener, this, _1, _2)); + if (NULL != m_idtag) + { + MyMetrics.RegisterListener(m_idtag, "*", std::bind(&canlog::MetricListener, this, _1)); + } + else + { + ESP_LOGE(TAG,"Cannot allocate memory - not registering metrics listener"); + }
int queuesize = MyConfig.GetParamValueInt("can", "log.queuesize",100); m_queue = xQueueCreate(queuesize, sizeof(CAN_log_message_t)); @ vehicle/OVMS.V3/components/can/src/canlog.cpp:374 @ canlog::canlog(const char* type, std::string format, canformat::canformat_serve_ canlog::~canlog() { MyEvents.DeregisterEvent(IDTAG); + if (NULL != m_idtag) + { + MyMetrics.DeregisterListener(m_idtag); + free((void*)m_idtag); + }
if (m_task) {
Another option is to change MyMetrics.RegisterListener so that it store *a copy* of the parameter, and does string comparison instead of a value comparison:
modified: vehicle/OVMS.V3/main/ovms_metrics.cpp ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── @ vehicle/OVMS.V3/main/ovms_metrics.cpp:1019 @ void OvmsMetrics::RegisterListener(const char* caller, const char* name, MetricC return; }
+ const char * caller_copy = (const char *)strdup(caller); + + if (NULL == caller_copy) + { + ESP_LOGE(TAG, "Problem registering metric %s for caller %s - insufficient memory",name,caller); + return; + } + MetricCallbackList *ml = k->second; - ml->push_back(new MetricCallbackEntry(caller,callback)); + ml->push_back(new MetricCallbackEntry(caller_copy,callback)); }
void OvmsMetrics::DeregisterListener(const char* caller) @ vehicle/OVMS.V3/main/ovms_metrics.cpp:1041 @ void OvmsMetrics::DeregisterListener(const char* caller) while (itc!=ml->end()) { MetricCallbackEntry* ec = *itc; - if (ec->m_caller == caller) + if (strcmp(ec->m_caller, caller) == 0) { + free((char *)ec->m_caller); itc = ml->erase(itc); delete ec; }
I've tested both approaches and they both solve my issue : no more crashes and memory corruption.
The second one is more "risky" (as it potentially impacts everyone) but I think that it should not cause much regression.
Let me know what you think - should I make a PR for this second approach ?
Regards,