[Ovmsdev] OBD poller module shell proposal
Michael Balzer
dexter at expeedo.de
Sat Mar 23 21:58:40 HKT 2024
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20240323/492d4cf5/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/20240323/492d4cf5/attachment-0001.sig>
More information about the OvmsDev
mailing list