[Ovmsdev] OBD poller module shell proposal
Michael Balzer
dexter at expeedo.de
Sat Apr 27 00:58:59 HKT 2024
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> *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>
> <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
>
> _______________________________________________
> 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/20240426/74f177af/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/20240426/74f177af/attachment-0001.sig>
More information about the OvmsDev
mailing list