[Ovmsdev] OBD poller module shell proposal
Michael Geddes
frog at bunyip.wheelycreek.net
Sat Apr 27 09:04:54 HKT 2024
That's great news, and I can certainly do this. My only concern is that
this branch as a whole contains a whole bunch of smaller pieces scattered
through it.
I have raised a number of pull requests over the last week that incorporate
various of the things I have added...
but if you are sure, I can certainly raise it as a single p/r with the
branch as-is.
If you accepted those recent p/r I would then rebase that whole branch
before doing the new p/r. But the result would be the same.
Just depends on how you want to track it. Again, I'll follow your lead.
//.ichael
On Sat, 27 Apr 2024 at 00:59, Michael Balzer via OvmsDev <
ovmsdev at lists.openvehicles.com> wrote:
> Michael,
>
> I've been using the new poller for two weeks now without issues. VWTP also
> works flawlessly.
>
> I'd say we merge this now, so more tests can be done on a wider variety of
> vehicles.
>
> Can you please update your pull request?
>
> Regards,
> Michael
>
>
> Am 09.04.24 um 04:35 schrieb frog at bunyip.wheelycreek.net:
>
> Thanks Michael,
>
> I’ve fixed the register/deregister and also fixed a few issues /
> dependencies for compiling when OVMS_COMP_POLLER is not selected.
>
>
>
> //.ichael
>
>
>
> *From:* OvmsDev <ovmsdev-bounces at lists.openvehicles.com>
> <ovmsdev-bounces at lists.openvehicles.com> *On Behalf Of *Michael Balzer
> *Sent:* Sunday, April 7, 2024 4:44 PM
> *To:* ovmsdev at lists.openvehicles.com
> *Subject:* Re: [Ovmsdev] OBD poller module shell proposal
>
>
>
> 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 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
>
>
> _______________________________________________
> 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/20240427/db37c1f2/attachment-0001.htm>
More information about the OvmsDev
mailing list