Build fixes, bug fixes and warning eliminations exposed when using CMake and ESP-IDF v5 #1096
Conversation
- incorrect source paths - missing source file - missing CMakeLists.txt for vehicle_zombie_vcu
Range reset can't have worked.
- mismatch between struct layout from mongoose and other code, causing data corruption and crash, due to MG_ENABLE_IPV6 being set only when compiling mongoose.c - missing volatile on variable across setjmp/longjmp - unsupported operation on unnecessarily volatile qualified variable - useless const on value cast - missing cmath for truncf/lroundf in some configuration - crash if config excludes some CAN busses - deprecated ADC define - unconditional dependency on Ethernet support which is not available on OVMS HW anyway - unconditional use of IPv6 related types and macros which are not available if IPv6 is disabled (unsupported by esp-wireguard). - use of IPv6-only internal structure of ip_addr_t, to check for ip6 addr type, even though the code only worked for IPv4 (e.g. IP2STR)
|
Welcome, Andreas, thanks for contributing! :-) A first note: removing the uncompressed web UI assets will break the web UI development environment (https://docs.openvehicles.com/en/latest/components/ovms_webserver/docs/index.html#web-ui-development-framework). Regards, |
|
Hi! Ah, I see. If you serve the files locally they are not available in their concatenated form. Let me see if I can do something about that use case. Would it be acceptable to make a wrapper python script that concatenates the files before starting the http.server? |
|
Or maybe it is acceptable to configure the project to create the files before starting the web dev server? Then I can just drop the one-step changes and let the intermediate concatenated files be generated too. Otherwise, I'll just keep the concatenated files in the repo. It's primarily the gz-files that I thought was good to get rid of. |
|
Sounds like introducing an additional place to take care of in case of asset changes, and also having the developer then need to deal with avoiding to accidentally check in the generated files. Why not simply keep the uncompressed files in the repository? |
|
The main issue I had with the gz files in the repo was that they changed depending on how they were built, always dirtying the tree. These changes have also been committed to the repo, se e.g. 7371c6f But also, having generated files in the repo, conactenated or compressed, places the burden on the developer to make sure the committed files are actually in sync with the source file and not change only one of them. This has also happened in the repo, e.g. 1811311 and 6201fbb . If concatenated files are generated, they should be in .gitignore, I think that solves the problem with accidental check-ins. Regardless, it's not a big deal and I can change that part to just sync the compression setting between CMake and makefiles if you prefer. That will fix the real issue I had with it. What do you think about the other changes? |
The CMake build and the legacy make build do not generate the same gzip files, due to different compression settings, so the worktree is getting dirty as soon as it's configured. While fixing this by using the higher compression also for make, take the opportunity to remove the compressed files from the repo. The intermediate concatenated files are kept to support the web UI dev env.
Extract the duplicated code to a helper method. Do the flag setting via a local variable to make it explicit when the volatile variable is accessed. The sensors state really should use atomics if it may be accessed from different contexts (threads/interrupts), or otherwise just remove volatile since it's not doing anything useful.
Fixes "error: conversion from 'bool' to 'const dbcNumber' is ambiguous".
|
The IPv6 case was broken anyway since it used IP2STR which only works with IPv4 addresses, so if DNS servers somehow got set to IPv6 address (not sure how), it would just print garbage. I figured since IPv6 addresses aren't printed elsewhere in the network_status() there's no point in doing it here. Not sure how to print IPv6 addresses and I can't really test it since I don't get one here. Otherwise that might be the best solution if we want to support IPv6 (I have it disabled intentionally in my build). Although the rest of the settings would need to be adapted as well. I removed the removal of concatenated assets, will push an update soon. |
nattgris
force-pushed
the
cmake_idf5_fixes
branch
from
82148ec to
287aa28
Compare
|
I rewrote the DNS print to use IP version agnostic ipaddr_ntoa_r(). However I don't know if it makes much sense to add it since no other IPv6 config is printed, such as the interfaces' addresses. Not even sure if the DNS address can get set to an IPv6 address. Not manually at least it can't since network.dns config is parsed as IPv4 only. Let me know if you want that patch added anyway. |
|
I leave that decision to you -- I also don't think IPv6 would work without further modifications. I just noted the dropped IPv6 case there because you guarded other places by preprocessor condition checks. |
Tested with ESP-IDF v5.0.7 in-car and build-only tested using the regular ESP-IDF version.