[Ovmsdev] Use of deallocated memory in a bunch of places!?

[Michael Geddes]

I can live with those astyle settings, so I'll include them.

I've been using  vim: tabstop=2 shiftwidth=2 expandtabs   (which is the
same as --indent=spaces=2 afaict) - so that's a relief.  Mixed tabs and
spaces are a pitb..

While I am used to delphi and so have been using the style that puts the
brace at the indent level l'm happy to go with the one you have chosen.
I really dislike that style with the brace indented.  While I still like
much about C++, especially the template engine, I do miss delphi enums,
sets and a few other features!

I'll download astyle and make it nice before I push up.  I've been making
lots of little commits as I'm going, but will probably just squash it into
one initial.

I'm wondering what I should do about my instrumented function calls!
For example - All my functions look like this at the moment (with XARM()
and XDISARM)

bool OvmsBatteryMon::checkStateChange()
{
  XARM("OvmsBatteryMon::checkStateChange");
  if (!m_dirty)
  {
    XDISARM;
    return false;
  }
  OvmsBatteryState newState = calc_state();
  bool res = newState != m_lastState;
  m_lastState = newState;
  m_dirty = false;
  XDISARM;
  return res;
}


These are Macros so they would process  to blank for the release.  The idea
is that if the object created by XARM() is not disarmed, it will cause a
log to happen. (ie if there's an unexpected exception).  Should I just
leave them in?

//.ichael


On Mon, 3 Oct 2022 at 14:26, Michael Balzer <dexter at expeedo.de> wrote:

> Thanks, Michael.
>
> > The main problem was the diversity of tab style.
>
> Yes… I see you even tried to fix indentation style where it seems to have
> been mixed unintentionally by the module developers ^_^
>
> Btw, I've begun including EditorConfigs and Artistic Style configs for
> newer modules, e.g.:
> -
> https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/blob/master/vehicle/OVMS.V3/components/zip/.editorconfig
> -
> https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/blob/master/vehicle/OVMS.V3/components/vehicle_vweup/src/.astylerc
>
> Regards,
> Michael
>
>
> Am 03.10.22 um 04:07 schrieb Michael Geddes:
>
> Pull request created.
>
> I hope it's ok.
> The main problem was the diversity of tab style.
>
> //.ichael
>
> On Mon, 3 Oct 2022 at 09:23, Michael Geddes <frog at bunyip.wheelycreek.net>
> wrote:
>
>> Not 8 issues.  8 Files that I found issues in, each with a bunch of them
>> - so around 50 all up.
>>
>> I did a vimgrep for lines starting with something like 'const char* ident
>> =' ish...
>>
>> Yeah, I agree, checking for IsDefined() is a lot less cludgy, so have
>> done that (yay for vim macros). Also the existing code didn't cope with -ve
>> values!
>>
>> I guess I better fork the project now and submit a pull request!! :)
>> //.ichael
>>
>> On Sun, 2 Oct 2022 at 22:01, Michael Balzer <dexter at expeedo.de> wrote:
>>
>>> Michael,
>>>
>>> oh, the dreadful c_str() trap once again… nice find.
>>>
>>> The deallocated heap buffer *can* have been reallocated easily, we're
>>> running on a preemptive OS and on two CPU cores.
>>>
>>> IOW, all of these need to be fixed. Sure there are only 8 of them?
>>>
>>> On your is_empty_metric() utility: may be I miss something, but why
>>> don't we simply use the metric's IsDefined() method?
>>>
>>> Regards,
>>> Michael
>>>
>>>
>>> Am 02.10.22 um 13:37 schrieb Michael Geddes:
>>>
>>> Hi all,
>>>
>>> I noticed some behavior on the *tpms* command I'd copied from the
>>> Kia/Kona code, which sent me down a small rabbit hole.
>>> What I was seeing was that the temperature was repeated instead of
>>> seeing the pressure and temperature.
>>>
>>> Even in vehicle.cpp, we can see code like this:
>>>
>>>   const char* range_est =
>>> StdMetrics.ms_v_bat_range_est->AsUnitString("-", rangeUnit, 0).c_str();
>>>   if (*range_est != '-')
>>>     writer->printf("Est. range: %s\n", range_est);
>>>
>>> My C++ is a bit rusty, however I believe that the 'const char*' won't
>>> hold the temporary std::string object returned by 'AsUnitString'.  To do
>>> that, I believe we need to assign to a reference (which I know will work).
>>> The problem is that the original code will mostly work (especially in cases
>>> like the above), as the deallocated heap memory won't have been reallocated.
>>>
>>>   const std::string& range_est =
>>> StdMetrics.ms_v_bat_range_est->AsUnitString("-", rangeUnit, 0);
>>>   if (!is_empty_metric(range_est))
>>>     writer->printf("Est. range: %s\n", range_est.c_str());
>>>
>>> Where I've defined in vehicle.h the following:
>>> inline bool is_empty_metric(const std::string &measure)
>>> {
>>>   return (measure == "") || (measure[0] == '-');
>>> }
>>>
>>> Will the above work functionally?  Would it be better to be:
>>>
>>> return (measure =="") || (measure == "-")
>>>
>>> This occurs in 8 files (outside my new Ioniq 5 files) that I have noted.
>>>
>>> I would make a separate commit for this and do a pull request for it,
>>>
>>> //.ichael
>>>
>>>
>>> _______________________________________________
>>> OvmsDev mailing listOvmsDev at lists.openvehicles.comhttp://lists.openvehicles.com/mailman/listinfo/ovmsdev
>>>
>>>
>>> --
>>> Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
>>> Fon 02333 / 833 5735 * Handy 0176 / 206 989 26
>>>
>>> _______________________________________________
>>> OvmsDev mailing list
>>> OvmsDev at lists.openvehicles.com
>>> http://lists.openvehicles.com/mailman/listinfo/ovmsdev
>>>
>>
> _______________________________________________
> OvmsDev mailing listOvmsDev at lists.openvehicles.comhttp://lists.openvehicles.com/mailman/listinfo/ovmsdev
>
>
> --
> Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
> Fon 02333 / 833 5735 * Handy 0176 / 206 989 26
>
> _______________________________________________
> OvmsDev mailing list
> OvmsDev at lists.openvehicles.com
> http://lists.openvehicles.com/mailman/listinfo/ovmsdev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20221003/263e9b2b/attachment.htm>