#Updating ArduinoJson Library

1 messages · Page 1 of 1 (latest)

prisma pond
#

I'm trying to upgrade ArduinoJson to version 7.4.2 PR, but running into clang-tidy issues complaining about a potential memory leak in the library itself. I'm surprised @hearty junco 's PR with the same version updated passes the tests.

#

Is it not actually running clang-tidy on the other failing components in bdraco's PR?

sturdy roost
#

possibly dumb question, but could an #ifdef be missing somewhere? I've seen that happen before and then it ends up checking code that wouldn't/shouldn't be checked for that particular platform 😁

prisma pond
#

bdraco seems to have "fixed" it with a commit to my PR making clang-tidy ignore external libraries 😅 This one is a false positive

sturdy roost
#

(just glancing over the PR, and it doesn't look like it, but maybe we need to sprinkle one in somewhere new?)

prisma pond
#

I was trying that earlier and couldn't find anything that would help

#

It was also annoying because I couldn't actually get my local clang-tidy runs to throw the error 😆

hearty junco
#

The other PR didn't change any of the json usage so it didn't do a clang-tidy run on it. It only bumped the deps.

prisma pond
#

Ah, makes sense

hearty junco
#

The system isn't expecting to run clang-tidy on external deps

#

one nit: the upgrade guide suggests using type-specific checks like is<int>() when you know the expected type. Can we narrow some of the is<JsonVariant>() ?

prisma pond
#

Yeah we probably could, I can try adjusting those quick

hearty junco
#

I haven't checked it for refactoring errors yet

#

I will do that after breakfast + walking pup

#

I thought the free was a problem but it looks like RAMAllocator::deallocate() doesn't actually use heap_caps_free() on ESP32 so I guess its not. Maybe we need an overload there to be able to call it without a size

#

how you have it is likely fine for now though

#

negative lookahead (?!...) is not supported in clang-tidy apparently

hearty junco
#

tried a more aggressive change. We try to make everything platformio as system headers already and exclude them but it doesn't look like its finding them all

#

also I can't reproduce it locally either

#

but it might just be because I'm on MacOS

prisma pond
hearty junco
#

oh its not working because they are errors not warnings

#

clang-tidy locally complains ```/Users/bdraco/esphome/esphome/components/mqtt/mqtt_datetime.cpp:23:25: error: consider replacing 'unsigned short' with 'uint16_t' [google-runtime-int,-warnings-as-errors]
23 | if (root["year"].is<unsigned short>()) {
| ^
/Users/bdraco/esphome/esphome/components/mqtt/mqtt_datetime.cpp:32:25: error: consider replacing 'unsigned short' with 'uint16_t' [google-runtime-int,-warnings-as-errors]
32 | if (root["hour"].is<unsigned short>()) {
| ^
/Users/bdraco/esphome/esphome/components/mqtt/mqtt_datetime.cpp:35:27: error: consider replacing 'unsigned short' with 'uint16_t' [google-runtime-int,-warnings-as-errors]
35 | if (root["minute"].is<unsigned short>()) {
| ^
/Users/bdraco/esphome/esphome/components/mqtt/mqtt_datetime.cpp:38:27: error: consider replacing 'unsigned short' with 'uint16_t' [google-runtime-int,-warnings-as-errors]
38 | if (root["second"].is<unsigned short>()) {

#

but doesn't seem to in the ci

prisma pond
#

I'll test that specifically then, I was following the types available from the ArduinoJson docs

hearty junco
#

I can't come up with a good solution to clang-tidy without disabling actual valid checks as well

#

so it looks like they will have to be disabled at each point

#

I'm working on that now

prisma pond
#

I just pushed a commit updating the type naming

hearty junco
#

its happy locally now

#

lets see what the CI says

#

If it passes, I'll merge it into my integration and retest on 8266, bk7200 and esp32

prisma pond
#

Thanks for looking at this! I was at a loss on how to proceed

hearty junco
#

if I missed any I'll fix them and push again after I actually eat breakfast 🙈

prisma pond
#

I had a couple wrong types (just too big of an unsigned int), added a couple missing nolint lines, and fixed a few formatting things due to the nolint messages. I pushed it, so hopefully next round we'll be closer 🥲

hearty junco
#

hopefully only one or two more rounds and I can start testing

#

I have a call in 30m

#

hoping to be able to test before than

#

really glad we made clang-tidy faster last week

#

trying to figure out where the nolint should go is not so great when you can't test locally.

#

hoping it will work at the top of the function for all of them

#

waiting for job 4 to finish and will push

#

🤞

#

seems ok on ESP8266, uses about 1.3K more RAM though

#

might just be jitter actually or heap fragmentation

#

reflashed again and its back to what it was before

#

ESP32 looks ok as well

#

testing bk7200 now

prisma pond
#

The author mentioned that they weren't going for the smallest CPU usage implementation with V7 but mainly for easy use

hearty junco
#

testing looks good on bk7200

#

not seeing any memory leaks either on any platform

#

so I think its just fighting with clang-tidy until its happy

#

out of time to add more. need to jump on this call

prisma pond
#

Thanks again for looking at it. I've been running it on the VPE for weeks without memory issues with the http request update and the resonate audio protocol running the whole time, so I think the memory management is fine

hearty junco
#

top of function didn't work so they need to be right before the assignment which means we need more than I would like but it is what it is

#

maybe changing the syntax to something like 193 - device_info[MQTT_DEVICE_CONNECTIONS][0][0] = "mac";
194 - device_info[MQTT_DEVICE_CONNECTIONS][0][1] = mac;
193 + JsonArray connections = device_info[MQTT_DEVICE_CONNECTIONS].to<JsonArray>();
194 + JsonArray connection = connections.add<JsonArray>();
195 + connection.add("mac");
196 + connection.add(mac);

#

not sure if thats exactly the same though

#

I think its the union type in the library that is tripping up clang-tidy

prisma pond
#

I wonder if we could convince the library's author to add the nolint lines. I think it would only need to be added in a couple of places in their source code.... They do run clang-tidy as part of their CI, but it's an old version, and it isn't performing the check we keep having problems with

hearty junco
#

I'd definately open an issue

#

worst they can say is no

prisma pond
#

Or could we just wrap the offending files with a nolintbegin and nolintend for this specific issue? It looks like we need it whenever we are accessing any element

#

I'll make an issue tomorrow morning, I'm running out of energy for the day 😅

hearty junco
#

Using NOLINTBEGIN and NOLINTEND would be cleaner -- its getting to be too many

#

let me do that

prisma pond
#

I have never been able to use new in my ESPHome code due to clang-tidy, it always suggested smart pointers. If there are other clang-tidy checks basically preventing using new in the first place, maybe this specific test is pointless

hearty junco
#

I switched all the multi occurrences to NOLINTBEGIN/NOLINTEND

cinder pike