Michael, The documentation I added was in the Google Doc OVMS Developer Guide: https://docs.google.com/document/d/1q5M9Lb5jzQhJzPMnkMKwy4Es5YK12ACQejX_NWEi... Is that document supposed to be replaced by the github-based Sphinx/RTD documentation that you mention? The Google Doc appears quite unfinished in that there are many blank pages between sections and the TOC is not updated. -- Steve On Tue, 22 Dec 2020, Michael Balzer wrote:

Thanks, Steve.

That reminds me of looking for a solution to generate RTD pages from the C++ sources, e.g. by sphinx-autodoc or similar. I prefer looking into the source, but have to admit the API documentation of the esp-idf is usable. We probably need to restructure most existing method & class comments and add the missing ones, but maintaining the API documentation would be easy afterwards.

Regards, Michael

Am 22.12.20 um 09:14 schrieb Stephen Casner:

...

At Mark's request I have added a description of how to add commands to the console command interpreter including the usage string syntax. This should be considered a draft at this point; questions and comments are welcome.

I added this under Vehicle Firmware Overview, but that might not be the right place. There is no general description of the core firmware.

-- Steve _______________________________________________ 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

Michael, OK, I converted my CLI documentation from the Google Doc into Sphinx. I added it at the top of the list of Developer Guides since the CLI is one of the first points of interaction for a developer (after the web installation wizard and configuration), but feel free to move it somewhere else that might be more appropriate. There may be markup conventions to adjust. I'm inclined to delete this section from the Google Doc now to avoid having changes diverge. Would you (and Mark) agree? I started with an automated conversion from the Google Doc source to rST markup, then manually cleaned it up. It wasn't too hard. Maybe that process could be use to move over the remained of the Google Doc. -- Steve On Sun, 27 Dec 2020, Michael Balzer wrote:

Steve,

yes, we're in the long-term migration of the developer documentation to Sphinx.

→ http://lists.openvehicles.com/pipermail/ovmsdev/2019-July/006196.html

The Google document still serves as an introduction for new firmware and hardware developers. New parts have been added to sphinx, but existing sections haven't been transformed yet. The parts in Sphinx are currently more targeted towards developers using the system, but just because they've been added after we decided to move to Sphinx.

Regards, Michael

Am 26.12.20 um 22:42 schrieb Stephen Casner:

...

Michael,

The documentation I added was in the Google Doc OVMS Developer Guide:

https://docs.google.com/document/d/1q5M9Lb5jzQhJzPMnkMKwy4Es5YK12ACQejX_NWEi...

Is that document supposed to be replaced by the github-based Sphinx/RTD documentation that you mention? The Google Doc appears quite unfinished in that there are many blank pages between sections and the TOC is not updated.

-- Steve

On Tue, 22 Dec 2020, Michael Balzer wrote:

...

Thanks, Steve.

That reminds me of looking for a solution to generate RTD pages from the C++ sources, e.g. by sphinx-autodoc or similar. I prefer looking into the source, but have to admit the API documentation of the esp-idf is usable. We probably need to restructure most existing method & class comments and add the missing ones, but maintaining the API documentation would be easy afterwards.

Regards, Michael

Am 22.12.20 um 09:14 schrieb Stephen Casner:

...

At Mark's request I have added a description of how to add commands to the console command interpreter including the usage string syntax. This should be considered a draft at this point; questions and comments are welcome.

I added this under Vehicle Firmware Overview, but that might not be the right place. There is no general description of the core firmware.

-- Steve _______________________________________________ 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

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

Michael,

thanks, looks good. Instead of removing the section from the Google doc, I suggest replacing it by a link to the RTD page.

Well, replacing it by a link would remove it, no? But I think perhaps what you meant is to add a link. I've done that.

* Please check for typos and grammar.

I found four errors, "to add ... are added", "Regiser", "the this" and "An variant". I consider myself a stickler for grammar so if you find other places to be problematic, please identify them specifically. Also I may tend to construct longer sentences than some editors would favor, so I have also tried to do a bit of wordsmithing.

* I opt to change the side note "explanatory text can be included" into a strong recommendation, as we still have many commands that lack explanations, and we should avoid the impression of that being the accepted standard. Users shall gain confidence about using a command by reading the command's usage info.

I don't think we need additional explanatory lines on all our commands, but I take your point. I've added a Note about this after the paragraph where the "usage" argument is introduced.

* I think it's worth mentioning RegisterCommand() tolerates duplicate registrations, allowing to forget about the init sequence. * The cleanup scheme (command deregistration) is missing.

Aha, my perspective has been from the C++ code. These comments are relevant to scripting, which is beyond my ken at this point. I've added a subsection and "TBA" where this discussion could go. Beyond the two points you mention, I think it may need part of the writeup you did for Mark when explaining how to manage this. Would you want to add that?

* An explanation of the "execute" and "validate" handler signatures probably also fits into the scope of this page.

Yes, since the developer will need to create them. I've added some explanations of the signatures and what the functions should do. Is there a clean way to put in references to source files? I could form an HTTP URL referencing the raw file in github, but that is unreliable. Google found this on the topic: https://github.com/readthedocs/readthedocs.org/issues/2926 Where there is a comment about how ESP-IDF documentation does this: http://esp-idf.readthedocs.io/en/latest/contribute/documenting-code.html#lin... And the code used to build the roles is here: https://github.com/espressif/esp-idf/blob/master/docs/idf_extensions/link_ro... -- Steve

On Tue, 29 Dec 2020, Michael Balzer wrote:

There certainly are commands that don't need explanations, but let me pick an example that evolved into the opposite: "metrics list" originally didn't need an explanation. Then the parameter "[<metric>]" was added, which already isn't self-explaining, as a user will interpret it to be a full metric name, but it actually is a substring to match the metric names against. With "[-ps]" we finally crossed the border of "you need to read the source code to know what this does". It isn't even clear from the usage template that "p" and "s" are separate options.

Agreed. I was surprised to discover only a few days ago that the concept of option flags had arisen. A Unix usage message would explain each option flag. That mechanism doesn't fit cleanly with our parsing scheme because it means the arg number of the value parameter varies depending upon the presence of optional flags. Obviously this can be programmed, but it seems like another mechanism might be needed to use option flags cleanly.

No, we'll add that to the scripting API sections when the feature is available. This section should focus on the C++ API, and I meant the C++ side as well:

* different core modules may add commands to shared roots (e.g. "obdii" is shared now by the vehicle and obd2ecu modules) * unloadable modules (e.g. vehicles) need to deregister their commands on unload

Aha, more that I didn't know. I know that one can dynamically select what vehicle type is active, but do we have loadable modules in the sense of a separate binary that gets merged with the core binary?

I personally think we should avoid linking to the source code from the documentation. A developer won't need that link, more appropriate is including condensed example snippets. We did so on the scripting API sections, for example: https://docs.openvehicles.com/en/latest/userguide/scripting.html#pubsub

The "literalinclude" directive can be used to keep longer / full examples along with the source code but still render them nicely in the documentation.

Thinking more about this, you are right. For my examples it would be appropriate to just insert a code snippet. -- Steve

Steve, very good to read and spot on now in my opinion. Regarding command options: for my latest command addition (obdii requests) I checked if we can use getopt_r(). Problems are a) it's not in the newlib included in esp-idf, and b) we also would need to change all handler signatures to allow getopt to modify the argv array. I also checked if some clever C++ getopt class is available but found none. I returned to implementing the option scanning the classical way, but took a note to implement a support class for this later on. Regards, Michael Am 30.12.20 um 03:23 schrieb Stephen Casner:

Michael,

OK, I've added a couple of paragraphs about duplicate registrations and unregistering. I also added code exerpts for the two source references and did some more wordsmithing and annotation cleanup. I think this is finished for now, but let me know if you see something that needs attention.

-- Steve

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

Steve, Am 01.01.21 um 02:34 schrieb Stephen Casner:

...

Regarding command options: for my latest command addition (obdii requests) I checked if we can use getopt_r(). Problems are a) it's not in the newlib included in esp-idf, and b) we also would need to change all handler signatures to allow getopt to modify the argv array. I also checked if some clever C++ getopt class is available but found none. Another option (pun intended) would be to stick more with the original parser design with trailing positional optional parameters rather than prefix option flags. Option flags are familiar to many of us developers, but that might not be the best choice for implementation here.

Positional optional parameters don't grow well for multiple options, force users to a fixed argument scheme and tend to produce hardly readable commands. I also think command options are for experienced/advanced users, and those are familiar with the concept. Extending the parser to support command argument templates would be an option, but I had the impression that would add a lot of complexity, and would only benefit a small set of (advanced) commands. (Side note: regarding command templates, I really liked the AmigaOS template scheme, IMO a shame it didn't get adopted more widely.)

Now, I'll admit that the obdii command has complex options that may need to be implemented as you have done. However, let me cite one counterexample:

Usage: event raise [-d<delay_ms>] <event>

This could have been done like homelink:

Usage: homelink <homelink> [<duration=1000ms>]

The latter form would be cleaner in that you wouldn't have to type the -d prefix. Also, the tab completion that I implemented before you added the delay option is now broken, although it would not be hard to fix.

Agreed for the case of a single option and a command that's not supposed to grow any new features. I did so on some of the CANopen commands. But imagine we need to add another numerical option to "event raise": nice and clean by prefix options, a usability nightmare with positional args. I wasn't aware of the tab completion on "event raise"… I normally use the command to raise custom events only. I'll fix that.

With the flexibility to have intermediate parameters now, using the validate function, it may fit reasonably to have the options handled by a subtree.

-- Steve

Do you mean a secondary option subtree triggered by the "-" prefix, inherited to the regular command subtree? That would allow simple tab completion on options as well… sounds like a nice idea. Regards, Michael -- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

Steve, Michael, et al, So I see the mention of obdii and it got me to pay attention... :) Regarding obdii "complexity"... I've always wondered why the obdii command only has 'ecu' as a second and only parameter. If obdii is essentially the command front-end to the obd2ecu task, that split is odd. What else was considered for the obdii framework? A few months ago there was a discussion on adding a bus speed parameter to the obd2ecu task start command. In general I'm not a fan of positional parameters, as they're not self-documenting, and parameter skipping is always hard to deal with. I also note that in terms of strings, there's no real difference between '-s', '--speed', and 'speed'. They're just strings to be matched. The value associated with that parameter, if any, would be included after a bit of white space. So, the proposal for adding a speed to the can bus selection would look like 'obdii ecu start can3 speed 500' (with kbits/sec implied). Alternatively, I can't imagine what else we'd add to that command, so just putting the '500' in there without an option prefix would work too. So, the use of option prefixes would be, um, "complexity driven"? All this said, I'm coming in from not really following where the command line parser discussion started. Greg Stephen Casner wrote:

Michael,

On Fri, 1 Jan 2021, Michael Balzer wrote:

...

Positional optional parameters don't grow well for multiple options, force users to a fixed argument scheme and tend to produce hardly readable commands. I also think command options are for experienced/advanced users, and those are familiar with the concept. As I said, I agree that there are some situations like obdii where the variability of operations is too complex for positional optional parameters. However, I don't consider a fixed argument scheme to be a bad thing for many of our commands. Do you consider the scheme I've set up for "location action" to be hardly readable?

...

Extending the parser to support command argument templates would be an option, but I had the impression that would add a lot of complexity, and would only benefit a small set of (advanced) commands. I agree that it makes sense to implement the flag options directly for those commands where it is appropriate.

...

Agreed for the case of a single option and a command that's not supposed to grow any new features. I did so on some of the CANopen commands. But imagine we need to add another numerical option to "event raise": nice and clean by prefix options, a usability nightmare with positional args. Depends whether the options are truly independent.

...

I wasn't aware of the tab completion on "event raise"... I normally use the command to raise custom events only. I'll fix that. Are those custom events not registered? RegisterEvent() adds events to m_map which makes them available for tab completion.

...

...

With the flexibility to have intermediate parameters now, using the validate function, it may fit reasonably to have the options handled by a subtree.

-- Steve Do you mean a secondary option subtree triggered by the "-" prefix, inherited to the regular command subtree?

That would allow simple tab completion on options as well... sounds like a nice idea. What I was referring to was that when there are multiple options needing their own argument schemes that can be handled by the subtree of OvmsCommand nodes that can follow an intermediate parameter, as in the "location action" command.

Tab completion wouldn't provide any benefit for single-letter option flags, but if options were words, as is the case for the different actions that can happen on a location, then it's useful.

I'm not sure if you were thinking of detatched command subtrees. I considered that idea when designing the validation scheme to allow intermediate parameters, but decided against it because I did not figure out a way to implement it cleanly including linking back up to the root for dynamic generation of the usage message.

-- Steve _______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

Greg, Am 03.01.21 um 04:15 schrieb Greg D.:

So I see the mention of obdii and it got me to pay attention...:)

Regarding obdii "complexity"... I've always wondered why the obdii command only has 'ecu' as a second and only parameter. If obdii is essentially the command front-end to the obd2ecu task, that split is odd. What else was considered for the obdii framework?

I don't know of any master plan for the "obdii" framework, but of course it's the natural place for all OBD2 tools to come. The general single OBD request command has been on my list since I added that to the Twizy code, always felt it should become a framework feature. As I recently got my new car (a Seat Mii, which is basically a VW e-Up), I now needed this command to start investigating the ECU registers on this car. While you technically could use the "re obdii" scanner for this, some fine people in the german forums already found all device addresses and most register addresses, so we're now already at the level of decoding the values. The next stuff I'd like to add in "obdii" is a general DTC tool, probably along with a new alert feature on DTC occurrence. But that won't reuse much of the Twizy code on this, as the Twizy had no support for standard OBD DTCs. Another concept fitting into "obdii" would be control for a full featured ISO-TP service process. The current implementation as the "poller" inside the vehicle class has it's limitations and oddities, although we've made quite some progress on this lately. A service worker concept as the one I've implemented for CANopen would be better. But that's more a long term todo. Regards, Michael -- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

Michael,

I've pushed the fix, but think it's still wrong to use the callback registration map to validate events without filtering out "*". While you technically may send "*" as an event, it isn't meant to be used that way, just gets added to the map because of the "any event" listeners.

I see. If raising event "*" should be avoided, though, the event_raise() function should check for that.

Regarding your question: custom events normally will not have a registered listener on the module. Event scripts do not register a native callback, the same applies to Javascript event subscriptions and events transported via the network to clients (e.g. web plugins or an MQTT listener). These are the main applications for custom events.

For someone writing new code who wants to listen for an event but does not know exactly how the event is written, the answer is not easy to find. I added "event list" to help with that, but based on what you say here that's not really valid or complete, either. -- Steve

Michael, On Mon, 4 Jan 2021, Michael Balzer wrote:

Am 04.01.21 um 08:32 schrieb Stephen Casner:

...

I see. If raising event "*" should be avoided, though, the event_raise() function should check for that.

That's an option for later, but not actually necessary, as it does no harm. But if users see "*" in the suggestions (!) of the command, they will interpret that as a defined feature.

It's not clear to me how you would like this to change. Should I remove the validate() function for event raise? -- Steve

On Mon, 4 Jan 2021, Michael Balzer wrote:

Steve,

Am 04.01.21 um 18:43 schrieb Stephen Casner:

...

It's not clear to me how you would like this to change. Should I remove the validate() function for event raise?

No, I would just remove the "*" from the suggestions. But no priority issue, it isn't broken, just a nice to have.

Done. Not hard, but not nice because that meant I could no longer use the NameMap<T> template and had to copy the code. -- Steve