Ah. You did merge those in. I'll rebase and out up a request. Michael On Sat, 27 Apr 2024, 09:04 Michael Geddes, <frog@bunyip.wheelycreek.net> wrote:
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@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@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@lists.openvehicles.com> <ovmsdev-bounces@lists.openvehicles.com> *On Behalf Of *Michael Balzer *Sent:* Sunday, April 7, 2024 4:44 PM *To:* ovmsdev@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-po... 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@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@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@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-po...
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@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@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@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@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@bunyip.wheelycreek.net> <frog@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________
OvmsDev mailing list
OvmsDev@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________
OvmsDev mailing list
OvmsDev@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________
OvmsDev mailing list
OvmsDev@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________
OvmsDev mailing list
OvmsDev@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@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev