<div dir="ltr"><div dir="ltr">Thanks so much for giving this a good run. </div><div>I totally missed that about the purpose of PollerStateTicker() damn.</div><div dir="ltr"><br></div><div>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.</div><div>In the original PR# 966 PollerSend() is an OvmsVehicle() member and as such this is a no-brainer. </div><div><br></div><div>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.</div><div><br></div><div>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 </div><div>entries in the list have been dealt with) rather than what will be at the beginning of each Primary tick.</div><div>Unless you have an objection - I'll pass in <font face="monospace">(canbus* bus)</font> to this - similar to PollRunFinished()</div><div><br></div><div>//.ichael</div><div><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 29 Mar 2024 at 21:51, Michael Balzer <<a href="mailto:dexter@expeedo.de" target="_blank">dexter@expeedo.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><u></u>
<div>
Michael,<br>
<br>
first feedback: testing this revealed some strange issues with
vehicle state changes apparently not being detected or reacted to
properly.<br>
<br>
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.<br>
<br>
The callback was explicitly meant to provide a means to change the
poller state just before the next poll would take place:<br>
<br>
<font face="monospace">/**<br>
* PollerStateTicker: check for state changes (stub, override with
vehicle implementation)<br>
* This is called by VehicleTicker1() just before the next
PollerSend().<br>
* Implement your poller state transition logic in this method,
so the changes<br>
* will get applied immediately.<br>
*/<br>
</font><br>
This change is already present in PR #966, so I'll delay merging
that.<br>
<br>
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.<br>
<br>
Regards,<br>
Michael<br>
<br>
<br>
<div>Am 26.03.24 um 08:46 schrieb Michael
Geddes:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Yeah wow. So close and yet so far.
<div><br>
</div>
<div>Fix pushed.</div>
<div><br>
</div>
<div>//.</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Sat, 23 Mar 2024 at 21:58,
Michael Balzer <<a href="mailto:dexter@expeedo.de" target="_blank">dexter@expeedo.de</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div> Michael,<br>
<br>
looking forward to test this, but trying to run your branch
with the VW e-Up leads to an early crash boot loop.<br>
<br>
Apparently `OvmsPollers::PollSetPidList()` doesn't check for
a NULL plist:<br>
<br>
<font face="monospace">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).<br>
1196 bool hasbus[1+VEHICLE_MAXBUSSES];<br>
1197 for (int i = 0 ; i <= VEHICLE_MAXBUSSES; ++i)<br>
1198 hasbus[i] = false;<br>
1199 <br>
1200 // Check for an Empty list.<br>
<font color="#d83a3a"><b>1201 if
(plist->txmoduleid == 0)</b></font><br>
1202 {<br>
1203 plist = nullptr;<br>
1204 ESP_LOGD(TAG, "PollSetPidList - Setting Empty
List");<br>
1205 }<br>
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).<br>
2273 return m_pollsignal;<br>
2274 }<br>
2275 void OvmsVehicle::PollSetPidList(canbus* bus,
const OvmsPoller::poll_pid_t* plist)<br>
2276 {<br>
2277 m_poll_bus_default = bus;<br>
2278 MyPollers.PollSetPidList(bus, plist,
GetPollerSignal());<br>
2279 }<br>
2280 #endif<br>
2281 <br>
2282 /**<br>
0x4023209e is in OvmsVehicleVWeUp::OBDInit()
(/home/balzer/esp/Open-Vehicle-Monitoring-System-3/vehicle/OVMS.V3/components/vehicle_vweup/src/vweup_obd.cpp:233).<br>
228 obd_state_t previous_state = m_obd_state;<br>
229 m_obd_state = OBDS_Config;<br>
230 <br>
231 if (previous_state != OBDS_Pause)<br>
232 {<br>
<font color="#d83a3a"><b>233
PollSetPidList(m_can1, NULL);</b></font><br>
234 PollSetThrottling(0);<br>
235 PollSetResponseSeparationTime(1);<br>
236 <br>
237 if
(StandardMetrics.ms_v_charge_inprogress->AsBool())</font><br>
<br>
<br>
Regards,<br>
Michael<br>
<br>
<br>
<div>Am 21.03.24 um 10:17 schrieb Michael Geddes:<br>
</div>
<blockquote type="cite">
<div dir="ltr">I've pushed up my working tree here: <a href="https://github.com/frogonwheels/Open-Vehicle-Monitoring-System-3/tree/new-poller" target="_blank">https://github.com/frogonwheels/Open-Vehicle-Monitoring-System-3/tree/new-poller</a>
<div>partly for feedback, and partly because I wanted it
backed up!<br>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>Of interest is really the final
components/poller/src/vehicle_poller* files and how
that has all come together.. I've added some
preliminary </div>
<div>documentation for that which I'm quite happy to
accept any critiques or requests for specific
information!</div>
<div><br>
</div>
<div>The Duktape metrics 'stale, age' methods are
probably ready for a p/r as-is.</div>
<div><br>
</div>
<div>//.ichael</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Sun, 17 Mar 2024 at
16:58, Michael Geddes <<a href="mailto:frog@bunyip.wheelycreek.net" target="_blank">frog@bunyip.wheelycreek.net</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr">Thanks Michael,
<div>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?</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>In my example the event notification looks like
this:<br>
void PollRunFinished(canbus *bus)<br>
{<br>
</div>
<div> m_runfinished_callback.Call(<br>
[bus](const std::string &name, const
PollCallback &cb)<br>
{<br>
cb(bus, nullptr);<br>
});<br>
</div>
<div> }</div>
<div><br>
</div>
<div>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.<br>
</div>
<div><br>
</div>
<div>Thoughts?</div>
<div><br>
</div>
<div>//.ichael</div>
<div>--------------8<
-----------------------------------</div>
<div>
<div><font face="monospace">/* Call-back register.<br>
* The list does not shrink which is fine for
this use-case.<br>
* Can be made inexpensively
threadsafe/re-entrant safe.<br>
*/<br>
template <typename FN><br>
class OvmsCallBackRegister<br>
{<br>
private:<br>
class CallbackEntry<br>
{<br>
public:<br>
CallbackEntry(const std::string
&caller, FN callback)<br>
{<br>
m_name = caller;<br>
m_callback = callback;<br>
}<br>
~CallbackEntry() {}<br>
public:<br>
std::string m_name;<br>
FN m_callback;<br>
};<br>
typedef
std::forward_list<CallbackEntry>
callbacklist_t;<br>
callbacklist_t m_list;<br>
public:<br>
~OvmsCallBackRegister()<br>
{<br>
}<br>
void Register(const std::string
&nametag, FN callback)<br>
{<br>
// Replace<br>
for (auto it = m_list.begin(); it !=
m_list.end(); ++it)<br>
{<br>
if ((*it).m_name == nametag)<br>
{<br>
(*it).m_callback = callback;<br>
return;<br>
}<br>
}<br>
if (!callback)<br>
return;<br>
for (auto it = m_list.begin(); it !=
m_list.end(); ++it)<br>
{<br>
if (!(*it).m_callback)<br>
{<br>
CallbackEntry &entry = *it;<br>
entry.m_name = nametag;<br>
entry.m_callback = callback;<br>
return;<br>
}<br>
}<br>
m_list.push_front(CallbackEntry(nametag,
callback));<br>
}<br>
void Deregister(const std::string
&nametag)<br>
{<br>
Register(nametag, nullptr);<br>
}<br>
typedef std::function<void (const
std::string &nametag, FN callback)>
visit_fn_t;<br>
void Call(visit_fn_t visit)<br>
{<br>
for (auto it = m_list.begin(); it !=
m_list.end(); ++it)<br>
{<br>
const CallbackEntry &entry = *it;<br>
if (entry.m_callback)<br>
visit(entry.m_name,
entry.m_callback);<br>
}<br>
}<br>
};</font><br>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, 14 Mar
2024 at 12:08, Mark Webb-Johnson <<a href="mailto:mark@webb-johnson.net" target="_blank">mark@webb-johnson.net</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>Michael,
<div><br>
</div>
<div>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).</div>
<div><br>
</div>
<div>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).</div>
<div><br>
</div>
<div>Regards, Mark<br id="m_-6530510750802225784m_7080410411435481348m_-1225568278149805507m_7550483307298702614m_-8888443040235954155lineBreakAtBeginningOfMessage">
<div><br>
<blockquote type="cite">
<div>On 14 Mar 2024, at 11:01 AM, Michael
Geddes <<a href="mailto:frog@bunyip.wheelycreek.net" target="_blank">frog@bunyip.wheelycreek.net</a>>
wrote:</div>
<br>
<div>
<div dir="ltr">
<div>Thanks Michael, Mark,</div>
<div><br>
</div>
<div>Sorry for not acknowledging
earlier.. this feedback is great;
I've just been cogitating on the
consequences. </div>
<div><br>
</div>
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).
<div><br>
</div>
<div>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..<br>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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).</div>
<div><br>
</div>
<div>//.ichael</div>
<div><br>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On
Mon, 11 Mar 2024 at 14:51, Michael
Balzer <<a href="mailto:dexter@expeedo.de" target="_blank">dexter@expeedo.de</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div> 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.<br>
<br>
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).<br>
<br>
Regards,<br>
Michael<br>
<br>
<br>
<div>Am 11.03.24 um 00:51 schrieb
Mark Webb-Johnson:<br>
</div>
<blockquote type="cite"> Michael,
<div><br>
</div>
<div>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).</div>
<div><br>
</div>
<div>If *only* within vehicle
framework, then putting it as
a sub-command under ‘vehicle’
seems sensible.</div>
<div><br>
</div>
<div>If more general purpose,
then perhaps look at ‘copen’
(component/canopen) as an
example.</div>
<div><br>
</div>
<div>Regards, Mark.<br id="m_-6530510750802225784m_7080410411435481348m_-1225568278149805507m_7550483307298702614m_-8888443040235954155m_393746923709381916lineBreakAtBeginningOfMessage">
<div><br>
<blockquote type="cite">
<div>On 10 Mar 2024, at
7:25 AM, Michael Geddes
<a href="mailto:frog@bunyip.wheelycreek.net" target="_blank"><frog@bunyip.wheelycreek.net></a>
wrote:</div>
<br>
<div>
<div dir="ltr">
<div>Hi all,</div>
<div><br>
</div>
<div>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).</div>
<div>My question is
not so much about
the
functionality and
status information,
but about the
location of the <b>poller</b> subcommand.
(See below). </div>
<div><br>
</div>
<div>Should 'vehicle'
be exclusively for
switching the
vehicle type?
Should the 'poller'
command be
top-level? Under
obdii? </div>
<div><br>
</div>
<div>Thoughts welcome.</div>
<div>If you do
vehicle poller
pause then the last
line reads 'Vehicle
OBD Polling is
paused'</div>
<div>//.</div>
<div>-------8<----------------------------------------</div>
<br>
<b>OVMS# vehicle ?</b><br>
Usage: vehicle
[list|module|poller|status]<br>
list
Show list of available
vehicle modules<br>
module
Set (or clear) vehicle
module<br>
poller
OBD polling status<br>
status
Show vehicle module
status<br>
<b>OVMS# vehicle
poller ?</b><br>
Usage: vehicle poller
[pause|resume]<br>
pause
Pause OBD Polling<br>
resume
Resume OBD Polling<b><br>
</b>
<div><b>OVMS# vehicle
poller</b><br>
OBD Polling running
on bus 1 with an
active list<br>
Time between polling
ticks is 1000ms with
1 secondary
sub-ticks<br>
Last poll command
received 1s (ticks)
ago.<br>
Vehicle OBD Polling
is running.<br>
</div>
</div>
_______________________________________________<br>
OvmsDev mailing list<br>
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank">OvmsDev@lists.openvehicles.com</a><br>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" target="_blank">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a><br>
</div>
</blockquote>
</div>
<br>
</div>
<br>
<fieldset></fieldset>
<pre>_______________________________________________
OvmsDev mailing list
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank">OvmsDev@lists.openvehicles.com</a>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" target="_blank">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a>
</pre>
</blockquote>
<br>
<pre cols="72">--
Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
Fon 02333 / 833 5735 * Handy 0176 / 206 989 26</pre>
</div>
_______________________________________________<br>
OvmsDev mailing list<br>
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank">OvmsDev@lists.openvehicles.com</a><br>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" rel="noreferrer" target="_blank">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a><br>
</blockquote>
</div>
_______________________________________________<br>
OvmsDev mailing list<br>
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank">OvmsDev@lists.openvehicles.com</a><br>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" target="_blank">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a><br>
</div>
</blockquote>
</div>
<br>
</div>
</div>
_______________________________________________<br>
OvmsDev mailing list<br>
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank">OvmsDev@lists.openvehicles.com</a><br>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" rel="noreferrer" target="_blank">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a><br>
</blockquote>
</div>
</blockquote>
</div>
<br>
<fieldset></fieldset>
<pre>_______________________________________________
OvmsDev mailing list
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank">OvmsDev@lists.openvehicles.com</a>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" target="_blank">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a>
</pre>
</blockquote>
<br>
<pre cols="72">--
Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
Fon 02333 / 833 5735 * Handy 0176 / 206 989 26</pre>
</div>
_______________________________________________<br>
OvmsDev mailing list<br>
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank">OvmsDev@lists.openvehicles.com</a><br>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" rel="noreferrer" target="_blank">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a><br>
</blockquote>
</div>
<br>
<fieldset></fieldset>
<pre>_______________________________________________
OvmsDev mailing list
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank">OvmsDev@lists.openvehicles.com</a>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" target="_blank">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a>
</pre>
</blockquote>
<br>
<pre cols="72">--
Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
Fon 02333 / 833 5735 * Handy 0176 / 206 989 26</pre>
</div>
_______________________________________________<br>
OvmsDev mailing list<br>
<a href="mailto:OvmsDev@lists.openvehicles.com" target="_blank">OvmsDev@lists.openvehicles.com</a><br>
<a href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev" rel="noreferrer" target="_blank">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a><br>
</blockquote></div></div>