[Ovmsdev] OBD poller module shell proposal
Michael Geddes
frog at bunyip.wheelycreek.net
Mon Apr 8 17:16:21 HKT 2024
Oh crap.
Yeah most definitely.
I can guess how that happened but damn.
Michael
On Sun, 7 Apr 2024, 16:44 Michael Balzer, <dexter at expeedo.de> wrote:
> Michael,
>
> I think this…
>
>
> https://github.com/frogonwheels/Open-Vehicle-Monitoring-System-3/blob/new-poller/vehicle/OVMS.V3/components/vehicle/vehicle.cpp#L358
> MyPollers.*RegisterRunFinished*(TAG,
> std::bind(&OvmsVehicle::PollerStateTickerNotify, this, _1, _2));
>
> …should be a call to MyPollers.RegisterPollStateTicker() instead, and
> OvmsVehicle::ShuttingDown() is missing a call to
> DeregisterPollStateTicker().
>
> Regards,
> Michael
>
>
> Am 31.03.24 um 01:44 schrieb Michael Geddes:
>
> I have pushed up changes for PR#966 as well as to the 'new-poller' branch
> that handle the PollerStateTicker()
>
> //.ichael
>
> On Sat, 30 Mar 2024 at 08:39, Michael Geddes <frog at bunyip.wheelycreek.net>
> wrote:
>
>> Thanks so much for giving this a good run.
>> I totally missed that about the purpose of PollerStateTicker() damn.
>>
>> I think I can call this on primary ticks only from PollerSend() itself
>> outside of the mutex. That would work - I'll get onto that.
>> In the original PR# 966 PollerSend() is an OvmsVehicle() member and as
>> such this is a no-brainer.
>>
>> That would mean it will get called from the Rx/Poller task rather than
>> the schedule (ticker1) task which is not necessarily a bad thing... but
>> different.
>>
>> I believe that for the Poller implementation though, this will need to be
>> an event similar to PollRunFinished() - which kind of has similar usage
>> but occurs between (effectively) poller ticks (ie when all the
>> entries in the list have been dealt with) rather than what will be at the
>> beginning of each Primary tick.
>> Unless you have an objection - I'll pass in (canbus* bus) to this -
>> similar to PollRunFinished()
>>
>> //.ichael
>>
>> On Fri, 29 Mar 2024 at 21:51, Michael Balzer <dexter at expeedo.de> wrote:
>>
>>> Michael,
>>>
>>> first feedback: testing this revealed some strange issues with vehicle
>>> state changes apparently not being detected or reacted to properly.
>>>
>>> A quick first check shows you have changed the way, the
>>> PollerStateTicker() hook works: it's now called independently from / as of
>>> the component ticker.1 registration sequence after the poller's primary
>>> send, rather than before.
>>>
>>> The callback was explicitly meant to provide a means to change the
>>> poller state just before the next poll would take place:
>>>
>>> /**
>>> * PollerStateTicker: check for state changes (stub, override with
>>> vehicle implementation)
>>> * This is called by VehicleTicker1() just before the next PollerSend().
>>> * Implement your poller state transition logic in this method, so the
>>> changes
>>> * will get applied immediately.
>>> */
>>>
>>> This change is already present in PR #966, so I'll delay merging that.
>>>
>>> Apart from that, the new poller seems to work normally, i.e. the polling
>>> scheme works and results are coming in, at least regarding ISOTP. I haven't
>>> tested VWTP yet.
>>>
>>> Regards,
>>> Michael
>>>
>>>
>>> Am 26.03.24 um 08:46 schrieb Michael Geddes:
>>>
>>> Yeah wow. So close and yet so far.
>>>
>>> Fix pushed.
>>>
>>> //.
>>>
>>> On Sat, 23 Mar 2024 at 21:58, Michael Balzer <dexter at expeedo.de> wrote:
>>>
>>>> Michael,
>>>>
>>>> looking forward to test this, but trying to run your branch with the VW
>>>> e-Up leads to an early crash boot loop.
>>>>
>>>> Apparently `OvmsPollers::PollSetPidList()` doesn't check for a NULL
>>>> plist:
>>>>
>>>> 0x40195b6f is in OvmsPollers::PollSetPidList(canbus*,
>>>> OvmsPoller::poll_pid_t const*, OvmsPoller::VehicleSignal*)
>>>> (/home/balzer/esp/Open-Vehicle-Monitoring-System-3/vehicle/OVMS.V3/components/poller/src/vehicle_poller.cpp:1201).
>>>> 1196 bool hasbus[1+VEHICLE_MAXBUSSES];
>>>> 1197 for (int i = 0 ; i <= VEHICLE_MAXBUSSES; ++i)
>>>> 1198 hasbus[i] = false;
>>>> 1199
>>>> 1200 // Check for an Empty list.
>>>> *1201 if (plist->txmoduleid == 0)*
>>>> 1202 {
>>>> 1203 plist = nullptr;
>>>> 1204 ESP_LOGD(TAG, "PollSetPidList - Setting Empty List");
>>>> 1205 }
>>>> 0x401a2675 is in OvmsVehicle::PollSetPidList(canbus*,
>>>> OvmsPoller::poll_pid_t const*)
>>>> (/home/balzer/esp/Open-Vehicle-Monitoring-System-3/vehicle/OVMS.V3/components/vehicle/vehicle.cpp:2278).
>>>> 2273 return m_pollsignal;
>>>> 2274 }
>>>> 2275 void OvmsVehicle::PollSetPidList(canbus* bus, const
>>>> OvmsPoller::poll_pid_t* plist)
>>>> 2276 {
>>>> 2277 m_poll_bus_default = bus;
>>>> 2278 MyPollers.PollSetPidList(bus, plist, GetPollerSignal());
>>>> 2279 }
>>>> 2280 #endif
>>>> 2281
>>>> 2282 /**
>>>> 0x4023209e is in OvmsVehicleVWeUp::OBDInit()
>>>> (/home/balzer/esp/Open-Vehicle-Monitoring-System-3/vehicle/OVMS.V3/components/vehicle_vweup/src/vweup_obd.cpp:233).
>>>> 228 obd_state_t previous_state = m_obd_state;
>>>> 229 m_obd_state = OBDS_Config;
>>>> 230
>>>> 231 if (previous_state != OBDS_Pause)
>>>> 232 {
>>>> *233 PollSetPidList(m_can1, NULL);*
>>>> 234 PollSetThrottling(0);
>>>> 235 PollSetResponseSeparationTime(1);
>>>> 236
>>>> 237 if (StandardMetrics.ms_v_charge_inprogress->AsBool())
>>>>
>>>>
>>>> Regards,
>>>> Michael
>>>>
>>>>
>>>> Am 21.03.24 um 10:17 schrieb Michael Geddes:
>>>>
>>>> I've pushed up my working tree here:
>>>> https://github.com/frogonwheels/Open-Vehicle-Monitoring-System-3/tree/new-poller
>>>> partly for feedback, and partly because I wanted it backed up!
>>>>
>>>> This incorporates about 10 pull/requests worth of work which is why I
>>>> didn't even add it as a draft, so I see this as taking some number of
>>>> months to push up.
>>>>
>>>> Of interest is really the final components/poller/src/vehicle_poller*
>>>> files and how that has all come together.. I've added some preliminary
>>>> documentation for that which I'm quite happy to accept any critiques
>>>> or requests for specific information!
>>>>
>>>> The Duktape metrics 'stale, age' methods are probably ready for a p/r
>>>> as-is.
>>>>
>>>> //.ichael
>>>>
>>>> On Sun, 17 Mar 2024 at 16:58, Michael Geddes <
>>>> frog at bunyip.wheelycreek.net> wrote:
>>>>
>>>>> Thanks Michael,
>>>>> I'll definitely add the config - pretty much sorted out the singleton
>>>>> (except for the change of directory). Just wondering if I should rebase
>>>>> this change down earlier or just keep it as the 'last change' that is
>>>>> making the final break. It might be a better transition that way?
>>>>>
>>>>> Anyway have a look at this little class below; we have a bunch of
>>>>> different implementations for an event register and came up with the
>>>>> OvmsCallBackRegister below.
>>>>>
>>>>> In my example the event notification looks like this:
>>>>> void PollRunFinished(canbus *bus)
>>>>> {
>>>>> m_runfinished_callback.Call(
>>>>> [bus](const std::string &name, const PollCallback &cb)
>>>>> {
>>>>> cb(bus, nullptr);
>>>>> });
>>>>> }
>>>>>
>>>>> I could possibly but it in main/ovms_utils.h ?? It could also use a
>>>>> std:map to implement it, though I think we save space this way and tbh the
>>>>> use case wouldn't be doing a lot of Register and Unregister which would be
>>>>> the slowest operations... I've made it not shrink the list size .... but
>>>>> again, not that important in the context.
>>>>>
>>>>> Thoughts?
>>>>>
>>>>> //.ichael
>>>>> --------------8< -----------------------------------
>>>>> /* Call-back register.
>>>>> * The list does not shrink which is fine for this use-case.
>>>>> * Can be made inexpensively threadsafe/re-entrant safe.
>>>>> */
>>>>> template <typename FN>
>>>>> class OvmsCallBackRegister
>>>>> {
>>>>> private:
>>>>> class CallbackEntry
>>>>> {
>>>>> public:
>>>>> CallbackEntry(const std::string &caller, FN callback)
>>>>> {
>>>>> m_name = caller;
>>>>> m_callback = callback;
>>>>> }
>>>>> ~CallbackEntry() {}
>>>>> public:
>>>>> std::string m_name;
>>>>> FN m_callback;
>>>>> };
>>>>> typedef std::forward_list<CallbackEntry> callbacklist_t;
>>>>> callbacklist_t m_list;
>>>>> public:
>>>>> ~OvmsCallBackRegister()
>>>>> {
>>>>> }
>>>>> void Register(const std::string &nametag, FN callback)
>>>>> {
>>>>> // Replace
>>>>> for (auto it = m_list.begin(); it != m_list.end(); ++it)
>>>>> {
>>>>> if ((*it).m_name == nametag)
>>>>> {
>>>>> (*it).m_callback = callback;
>>>>> return;
>>>>> }
>>>>> }
>>>>> if (!callback)
>>>>> return;
>>>>> for (auto it = m_list.begin(); it != m_list.end(); ++it)
>>>>> {
>>>>> if (!(*it).m_callback)
>>>>> {
>>>>> CallbackEntry &entry = *it;
>>>>> entry.m_name = nametag;
>>>>> entry.m_callback = callback;
>>>>> return;
>>>>> }
>>>>> }
>>>>> m_list.push_front(CallbackEntry(nametag, callback));
>>>>> }
>>>>> void Deregister(const std::string &nametag)
>>>>> {
>>>>> Register(nametag, nullptr);
>>>>> }
>>>>> typedef std::function<void (const std::string &nametag, FN
>>>>> callback)> visit_fn_t;
>>>>> void Call(visit_fn_t visit)
>>>>> {
>>>>> for (auto it = m_list.begin(); it != m_list.end(); ++it)
>>>>> {
>>>>> const CallbackEntry &entry = *it;
>>>>> if (entry.m_callback)
>>>>> visit(entry.m_name, entry.m_callback);
>>>>> }
>>>>> }
>>>>> };
>>>>>
>>>>> On Thu, 14 Mar 2024 at 12:08, Mark Webb-Johnson <mark at webb-johnson.net>
>>>>> wrote:
>>>>>
>>>>>> Michael,
>>>>>>
>>>>>> I suggest that if it is a separate component then better to move it
>>>>>> to it’s own component directory (just as canopen is done).
>>>>>>
>>>>>> For completeness, I suggest it would also be good to include
>>>>>> a CONFIG_OVMS_COMP_* sdkconfig (default: yes), and put that as a
>>>>>> requirement for your component (as well as for any vehicle doing polling, I
>>>>>> guess).
>>>>>>
>>>>>> Regards, Mark
>>>>>>
>>>>>> On 14 Mar 2024, at 11:01 AM, Michael Geddes <
>>>>>> frog at bunyip.wheelycreek.net> wrote:
>>>>>>
>>>>>> Thanks Michael, Mark,
>>>>>>
>>>>>> Sorry for not acknowledging earlier.. this feedback is great; I've
>>>>>> just been cogitating on the consequences.
>>>>>>
>>>>>> I still have the Poller hanging onto the vehicle by a thread so I
>>>>>> should just cut the thread making the Poller a separate singleton (it's
>>>>>> still embedded in the vehicle class for now with a small interface joining
>>>>>> them).
>>>>>>
>>>>>> If I do, does it need to get moved to a new directory or can it stay
>>>>>> in the vehicle/ directory? The file vehicle_poller.cpp (and the _isotp and
>>>>>> vwtp parts to it) are still pretty much as they were with only a change in
>>>>>> class name..
>>>>>>
>>>>>> I think I just need the poller to get its own values of m_can1 etc
>>>>>> and provide a different way of getting the feedback results.
>>>>>>
>>>>>> I also need to make sure I'm not cutting off the 'vehicle' class'
>>>>>> access to non-solicited messages (ie stuff that is just on the bus).
>>>>>>
>>>>>> //.ichael
>>>>>>
>>>>>>
>>>>>> On Mon, 11 Mar 2024 at 14:51, Michael Balzer <dexter at expeedo.de>
>>>>>> wrote:
>>>>>>
>>>>>>> Actually, separating the poller from the vehicle was part of the
>>>>>>> plan of reworking it into a job/worker architecture. I see no reason the
>>>>>>> generalized poller would need to remain coupled to the vehicle.
>>>>>>>
>>>>>>> That's why I placed the OBD single request command in the "obdii"
>>>>>>> hierarchy (although a more proper naming would have been e.g. "isotp", but
>>>>>>> changing the name or having both would confuse users -- and meanwhile the
>>>>>>> poller also supports a non-ISO TP variant).
>>>>>>>
>>>>>>> Regards,
>>>>>>> Michael
>>>>>>>
>>>>>>>
>>>>>>> Am 11.03.24 um 00:51 schrieb Mark Webb-Johnson:
>>>>>>>
>>>>>>> Michael,
>>>>>>>
>>>>>>> It depends on whether the poller can *only* be used in the vehicle
>>>>>>> class or if it is a framework all by itself (for example with commands to
>>>>>>> manually poll specific PIDs, etc).
>>>>>>>
>>>>>>> If *only* within vehicle framework, then putting it as a sub-command
>>>>>>> under ‘vehicle’ seems sensible.
>>>>>>>
>>>>>>> If more general purpose, then perhaps look at ‘copen’
>>>>>>> (component/canopen) as an example.
>>>>>>>
>>>>>>> Regards, Mark.
>>>>>>>
>>>>>>> On 10 Mar 2024, at 7:25 AM, Michael Geddes
>>>>>>> <frog at bunyip.wheelycreek.net> <frog at bunyip.wheelycreek.net> wrote:
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I know some of this (especially for the status) functionality is
>>>>>>> predicated on code that's not gone up yet - however this is allowing
>>>>>>> 'pause' and 'resume' of the poller (which has been merged).
>>>>>>> My question is not so much about the functionality and status
>>>>>>> information, but about the location of the *poller* subcommand.
>>>>>>> (See below).
>>>>>>>
>>>>>>> Should 'vehicle' be exclusively for switching the vehicle type?
>>>>>>> Should the 'poller' command be top-level? Under obdii?
>>>>>>>
>>>>>>> Thoughts welcome.
>>>>>>> If you do vehicle poller pause then the last line reads 'Vehicle
>>>>>>> OBD Polling is paused'
>>>>>>> //.
>>>>>>> -------8<----------------------------------------
>>>>>>>
>>>>>>> *OVMS# vehicle ?*
>>>>>>> Usage: vehicle [list|module|poller|status]
>>>>>>> list Show list of available vehicle modules
>>>>>>> module Set (or clear) vehicle module
>>>>>>> poller OBD polling status
>>>>>>> status Show vehicle module status
>>>>>>> *OVMS# vehicle poller ?*
>>>>>>> Usage: vehicle poller [pause|resume]
>>>>>>> pause Pause OBD Polling
>>>>>>> resume Resume OBD Polling
>>>>>>> *OVMS# vehicle poller*
>>>>>>> OBD Polling running on bus 1 with an active list
>>>>>>> Time between polling ticks is 1000ms with 1 secondary sub-ticks
>>>>>>> Last poll command received 1s (ticks) ago.
>>>>>>> Vehicle OBD Polling is running.
>>>>>>> _______________________________________________
>>>>>>> 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
>>>>>>>
>>>>>> _______________________________________________
>>>>>> OvmsDev mailing list
>>>>>> OvmsDev at lists.openvehicles.com
>>>>>> http://lists.openvehicles.com/mailman/listinfo/ovmsdev
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> 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
>>>>
>>>
>>> _______________________________________________
>>> 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/20240408/ccdb4cea/attachment-0001.htm>
More information about the OvmsDev
mailing list