search

jomjol / AI-on-the-edge-device

Easy to use device for connecting "old" measuring units (water, power, gas, ...) to the digital world
https://jomjol.github.io/AI-on-the-edge-device-docs/
5.26k stars 580 forks source link

update platformIO to 6.7.0 (ESP IDF 5.2.1) #3098

Closed caco3 closed 4 weeks ago

caco3 commented 1 month ago

TODO

Slider0007 commented 1 month ago

@caco3

components/jomjol_flowcontroll/ClassFlow.h:49:24: error: 'virtual std::string ClassFlow::getReadout()' was hidden [-Werror=overloaded-virtual=] 49 | virtual string getReadout();

A possible correction for this issue can be found here: https://github.com/Slider0007/AI-on-the-edge-device/pull/160 --> I've just removed that part because it's not used anyway

components/jomjol_fileserver_ota/miniz/miniz.c:6250:34: warning: comparison is always false due to limited range of data type [-Wtype-limits] 6250 | if (((mz_uint64)buf_size > 0xFFFFFFFF) || (uncomp_size > 0xFFFFFFFF))

IMHO this would be only critical for really large files.

caco3 commented 4 weeks ago

@Slider0007 Thanks for the tip. I adapted your suggestion. But how do we know for sure it is unused?

Looking on your PR I see that you also changed other things (NTP). I would prefer to separate the upgrade and the NTP feature to make troubleshooting easier.

And for the warning, I agree that it should be ok. Once they have a solution, we can upgrade.

Slider0007 commented 4 weeks ago

But how do we know for sure it is unused?

As long class flowpostprocessing is not NULL, the removed code part never gets reached. And flowpostprocessing cannot be disabled in our environment. I assume this is some code jomjol used in the past.

Looking on your PR I see that you also changed other things (NTP)

This is not necessary anymore for jomjol software (already included in last migration from 6.3.2 to 6.5 -> https://github.com/jomjol/AI-on-the-edge-device/pull/2770) because you only migrate from 6.5 to 6.7. I migrated from 6.3.2 to 6.7, though...

-> In summary, IMHO as long it compiles no more changes are required.

caco3 commented 4 weeks ago

It works on my devices. If you agree with the changes you can merge it.