[Ovmsdev] OBD poller module shell proposal
Michael Balzer
dexter at expeedo.de
Fri Mar 29 21:51:26 HKT 2024
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20240329/6f757c93/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/20240329/6f757c93/attachment-0001.sig>
More information about the OvmsDev
mailing list