[Ovmsdev] OBD poller module shell proposal
Michael Balzer
dexter at expeedo.de
Sun Apr 7 16:43:40 HKT 2024
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>
>>>>>> <mailto: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 list
>>>>> OvmsDev at lists.openvehicles.com
>>>>> http://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 list
>>> OvmsDev at lists.openvehicles.com
>>> http://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
>
> --
> 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
--
Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
Fon 02333 / 833 5735 * Handy 0176 / 206 989 26
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20240407/0aec32d1/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 203 bytes
Desc: OpenPGP digital signature
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20240407/0aec32d1/attachment-0001.sig>
More information about the OvmsDev
mailing list