Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge-v24.4.12 #923

Merged
merged 21 commits into from
Apr 29, 2024
Merged

Merge-v24.4.12 #923

merged 21 commits into from
Apr 29, 2024

Conversation

helgeerbe
Copy link
Collaborator

No description provided.

@helgeerbe helgeerbe marked this pull request as draft April 25, 2024 19:03
@helgeerbe helgeerbe requested a review from schlimmchen April 25, 2024 19:04
@helgeerbe
Copy link
Collaborator Author

This is the first version, which compiles.

Sorry, but this time this is not an easy task due to the changes in the Json libs.

At the moment, we have the vue lint errors and we can't do the linking

Linking .pio/build/d1_mini_esp32/firmware.elf
Retrieving maximum program size .pio/build/d1_mini_esp32/firmware.elf
Checking size .pio/build/d1_mini_esp32/firmware.elf
Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
RAM:   [==        ]  24.3% (used 79660 bytes from 327680 bytes)
Flash: [==========]  100.1% (used Error: The program size (1967293 bytes) is greater than maximum allowed (1966080 bytes)
1967293 bytes from 1966080 bytes)
*** [checkprogsize] Explicit exit, status 1

@schlimmchen
Copy link
Member

generic
Error: The program size (1966925 bytes) is greater than maximum allowed (1966080 bytes)

generic_esp32
Error: The program size (1966921 bytes) is greater than maximum allowed (1966080 bytes)

d1_mini_esp32
Error: The program size (1967133 bytes) is greater than maximum allowed (1966080 bytes)

It seems we are at most 1k Bytes over. Let's hope we can carve that out and get away with it one more time. I'll start by adopting 8add226.

The vue linter errors are not a big deal. At least I think so. Thomas had to fix a bunch as well and shows us in de156ef how to ignore lines if we decide that fixing the linter error is not worth our time.

@helgeerbe
Copy link
Collaborator Author

We can change the partitions:

But this would be a breaking change for 4 MB Devices (not OTA update)

# Name,   Type, SubType, Offset,   Size,    Flags
nvs,      data, nvs,     0x9000,   0x5000,
otadata,  data, ota,     0xe000,   0x2000,
app0,     app,  ota_0,   0x10000,  0x1F0000,
app1,     app,  ota_1,   0x200000, 0x1F0000,
spiffs,   data, spiffs,  0x3F0000, 0x10000,

@schlimmchen
Copy link
Member

We can change the partitions:

If we went there, I would push for a much more aggressive approach: drop support for 4MB devices altogether and enlarge each app partition to roughly 4MB (expect 8MB flash total minimum, like ESP32-S3 on OpenDTU FUSION).

This will be a major disruption and break upgrading from stock OpenDTU as well. I hope we can postpone such drastic measures a little longer.

I am targeting the FirebaseJson Library. Remember when someone took it out and saved ~20k of Flash? We had to put it back in because the HTTPS+JSON power meter broke. I hope that ArduinoJson 7 has more capabilities in this regard. And if users have to adjust the setting to be compatible with ArduinoJson 7 rather than FirebaseJson, then I think this is acceptable.

There might be more options to save code. Do you have more ideas?

@schlimmchen
Copy link
Member

With adopting WebApiClass::parseRequestData() we are already back in business, as it seems:

Checking size .pio/build/d1_mini_esp32/firmware.elf
Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
RAM:   [==        ]  24.3% (used 79532 bytes from 327680 bytes)
Flash: [==========]  99.9% (used 1963133 bytes from 1966080 bytes)

@helgeerbe
Copy link
Collaborator Author

Beside Firebase I would check, if we can get rid of Cypto lib. If I remember right, we just need SHA256. So there might be a more light wise approach.

Don't know if there is a complier optimization level "Optimize for size" for platformio.

Last thing.
Use a small OTA loader, as second binary. So we don't waste the memory to keep a complete second bin.

@schlimmchen
Copy link
Member

I would check, if we can get rid of Cypto lib

Is TLS support (MQTT and HTTP clients) implemented elsewhere? And I would assume, that if only SHA256 functionality is used, only little code is actually linked in. Let's find out.

Use a small OTA loader, as second binary. So we don't waste the memory to keep a complete second bin.

Yes. This also means there will be a breaking change, but that's more a less a given in the long run. The big disadvantage with this approach is that we need to build that second smaller binary. (You are talking about the "tasmota-minimal" approach, right?) That will be a headache in the code, the web app (which needs to be a different one), and the build process.

I also thought we might want to drop the second binary altogether. If it gets corrupted (OTA update), the flash must be rewritten using the USB interface/serial loader. At least for the 4MB devices. For other devices, we might keep the approach with two binaries, but still enlarge the bin size.

I just scrolled Wikipedia and now I think that requiring ESP32 with at least 8MB of flash will be very harsh. The default is 4MB, and most people will end up with such a device, and they will be frustrated to find out that they can't use such a board at all.

Don't know if there is a complier optimization level "Optimize for size" for platformio.

I checked some time ago, it's already -Os (which is "optimize for size"). Which I still did not try is find some sort of memory analyzer, which tells us what significant chunks of code are coming from which source so we can target it directly.

@schlimmchen
Copy link
Member

Missed one opportunity to use WebApiClass::parseRequestData(). Amended the respective commit and force-pushed.

@swingstate
Copy link

Hi @schlimmchen,

I'm exploring the possibility of discontinuing support for devices with 4MB flash. Do you think increasing the minimum to 8MB would pave the way for new features or advantages in the near future? It might be worth communicating this potential change widely, such as in changelogs, the wiki, GitHub page, and other prominent spaces. For example: "Starting January 1st, 2025, ODTUoB will no longer support 4MB flash. Further details can be found here."

Personally, I believe phasing out support for these smaller devices could catalyze further development. What are your thoughts?

Best regards,

Copy link
Member

@schlimmchen schlimmchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helge's merge conflict resolution makes sense.

@schlimmchen
Copy link
Member

@helgeerbe I want to go ahead and merge this to development, so we can start testing. I need #933 as of yesterday, so I will slip that in right now and then merge this PR.

@schlimmchen schlimmchen marked this pull request as ready for review April 29, 2024 19:15
@schlimmchen schlimmchen merged commit 4cf596e into development Apr 29, 2024
10 checks passed
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 30, 2024
@schlimmchen schlimmchen deleted the merge-v24.4.12 branch June 1, 2024 15:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants