SmartEQ - Compilation fails, DDT4All components and general features

[zbchristian]

First, I like to thank @zorgms for the substantial update of the Smart EQ code! Great work!

• There is a small compilation issue:
  If plugins are disabled, the header ovms_plugins.h is not available and an error is thrown. Since the header is not needed in vehicle_smarteq.cpp, it can just be deleted. So, an easy fix.

Here some more general issues, I have with the code. Please take these as positive feedback for this great work!

• Concerns about the ddt4all support
  Nice work, to collect all the information.
  I have been using DDT4All together with an ELM237 module to enable/disable Control Unit features of my Smart EQ, but this is quite an invasive operation and goes way beyond a Monitoring device.
  The implementation looks currently very experimental, especially, because the access is done through the valet command on the OVMS console.
  My take on this is, that this is a nice feature, but definitely not for everyone. The user has to understand the consequences, when changing the paramerters of the Control Units in the car.
  To be honest, I do not want this code to be present in my OVMS device. I prefer to do these type of operations under very controlled conditions with specialized hard- und software.
  So, my proposal is, that this code is only available, if its explicitly enabled via menuconfig. I created a corresponding branch in the forked repo https://github.com/zbchristian/Open-Vehicle-Monitoring-System-3-Lilygo-Support.git (branch Smarteq_ddt4all_option)

• Included features not related to the Smart EQ
  The code includes now features, which deal with OVMS general topis: power management, cellular modem operation, OVMS server V2 connection.
  There might be good reasons for the handling of these gerenal topics, but I think, that these features would better be contributed to the corresponding parts of the general OVMS code. This would be beneficial for all users.
  Since I am afraid, that there could be interferences with the general OVMS code, I currently disable these parts of the code.

[zorgms]

DDT4all can only be used if it is activated. I don't believe in patronizing people by requiring the function to be activated additionally for firmware compilation. Especially since it excludes many people from using it and unnecessarily complicates things.

Since the EQ doesn't have a Valet function, I used it. I've already considered possibly changing this to xsq ddt4all.

As for the other non-smart EQ-specific functions, the question is, can all vehicles implemented in OVMS handle these functions? All functions have been tested by many smart EQ owners with OVMS, and all functions can be optionally switched on/off via features.

[zorgms]

OK, I found and fixed a logic error. Additionally, ddt4all can only be activated via command for 5 minutes. I also changed ddt4all commands from unvalet to xsq ddt4all.

[dexterbg]

My 2 cents: the "DDT4ALL" command feature is basically just a wrapper for the underlying OBD requests defined by the DDT. Without that command, you can still do all the ECU register writes by using the generally available OBD request command and reading the DDT (which is easy to read with just basic knowledge). And by using low level OBD access you can even write invalid values, which is avoided by the wrapper by assigning unique numbers to the exposed command/value combinations.

So the wrapper is basically an added layer to avoid bricking your ECUs. That's the equivalent of the Twizy tuning macro commands. On the Twizy you also can change any SEVCON register using the low level CANopen commands. Using the macro commands restricts the usable tuning values to (mostly) sane combinations.

While using the OVMS for ECU tuning exceeds the original "monitoring" aspect, I'm perfectly fine with that, as long as the user has control over this. I purposefully introduced the OBD and CANopen low level commands to make ECU configuration available to all users, because quite some bad decisions were made by the manufacturers on default configurations, and if we can give users control over such things, we should do so.

Remapping some V2 standard MP command is really no longer necessary though, since all Apps now support text commands. Zorg seems to have addressed this already by the latest PR.

What's really missing is a user documentation on the available "DDT4ALL" commands and their effects & caveats.

Full ACK on shifting general extensions into the framework, as I already suggested on the PRs. But some of them looked like hacks or really unnecessary attempts to work around some probably only perceived issue, or some temporary issue from basically all Smart owners currently running the "edge" firmware. To consider a function for framework inclusion, I ask for this to be generally applicable, isolated, well implemented and well documented.

Regards,
Michael

[zbchristian]

@zorgms, thanks for the fixes.
Concerning your question about the general features and the support by all vehicles. Except for the 12V recharge feature, all others are just cellular modem/GPS and network related. There is no dependence on the actual vehicle. All the needed basic functionality is handled by the framework.

Ok, Michael, good to know, that the ddt4all option is in principle acceptable. I am still not a fan of this type of code ;-)