Skip to content

Conversation

@blattm
Copy link
Contributor

@blattm blattm commented Nov 19, 2025

Hi!
This PR aims to improve the readability of main.c by separating parsing logic from message handling.
Although the code is already separated, it's not perfect, and there is still work to be done. There are, for example, too many variables being passed in function calls for my taste.

I'm planning to write a read_byte function that the parser can call. If would fetch data from the network when the buffer is empty. This would allow us to eliminate the switch / case and make many variables locally scoped. I would use error codes to detect whether a reset (corresponding to the outermost while loop of the ´http_get_task`) is necessary.

It might also make sense to further modularize the message-handling side (e.g., splitting out time sync, codecs, etc.), but that can easily be done in a later PR.

return PARSER_INCOMPLETE;
}

parser_return_state_t parse_sever_settings_message(snapcast_custom_parser_t *parser,
Copy link
Owner

Choose a reason for hiding this comment

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

You could put all those variables passed to the functions in a struct/union combination.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I will do this, once the parser is simplified. I think I will be able to remove a lot of parameters corresponding to internal parser state first.

@CarlosDerSeher
Copy link
Owner

CarlosDerSeher commented Nov 26, 2025

Before you continue your effort maybe we should check if netconn api is really the best thing to use. There have been some packet delivery issues on my last tests and I am uncertain where this comes from. Also espressif says netconn is less resource intensive but when asking chatGPT I get this for an answer

Why does espressif say in it's documentation netconn is less resource intensive than bsd sockets as those are built on top of netconn in lwip

**ChatGPT:**
Great question — and the confusion is completely justified.
The apparent contradiction comes from how lwIP was originally designed versus how ESP-IDF actually implements BSD sockets today.

Below is the precise reason why Espressif says “netconn is less resource-intensive than BSD sockets”, and why this statement is both true in theory and false in practice depending on context.

✅ 1. In upstream lwIP, BSD sockets are layered on top of netconn
In vanilla lwIP (the version without Espressif modifications):

sockets API → netconn API → raw lwIP (PCB)
Therefore:

A BSD socket contains a struct lwip_sock

That struct contains a pointer to a struct netconn

The netconn contains mailboxes, semaphores, and state

→ So sockets add one extra layer of abstraction on top of netconn

In pure lwIP, this means:

BSD sockets = netconn + extra wrapper
→ So yes, sockets use more RAM than netconn.

This is the context Espressif is referencing.

❗ 2. BUT ESP-IDF modifies lwIP — and the layering is not identical anymore
Espressif has patched lwIP to integrate:

esp_netif

TCP/IP task changes

Optimized memory pools

mbedTLS handling

Event loop integration

In ESP-IDF:

✔ The sockets layer still uses netconn
BUT
✔ Espressif optimized out some netconn overhead for the socket layer
✔ And the socket layer has its own optimizations

In other words:

BSD sockets in ESP-IDF do NOT incur all of the overhead that upstream lwIP documented.

This is why users notice that:

BSD sockets behave more predictably

Netconn often uses more RAM for real-world workloads

Netconn copies data more often

Socket recv() can reuse a single buffer while netconn buffers queue up pbufs/netbufs

Espressif’s documentation is talking about theoretical upstream lwIP architecture, not actual ESP-IDF runtime memory usage.

So I am abit confused now and I think a comparison of the two should be done by logging max heap usage while reception. Maybe we can integrate this into http_task and just drop all the data and do the same using sockets

@blattm
Copy link
Contributor Author

blattm commented Jan 23, 2026

I’ve significantly reduced the number of parameters passed to process_data. Many of the previous parameters were only used to forward specific parsing results to the handling functions.

These values now live in much smaller scopes: either directly inside process_data or within the parsing functions where they logically belong.

Strongly reducing the parameter count further would likely mean grouping values with little semantic connection into a shared struct. I’m not sure that would improve readability or maintainability. But I may also be missing what kind of refactoring you have in mind. If you’d like me to continue in this direction, a concrete example of the struct or grouping you’re thinking of would really help.

Let me know if the current state works for you. If so, I’ll resolve the conflicts and get things wrapped up 🙂

@blattm blattm marked this pull request as ready for review January 23, 2026 17:37
@CarlosDerSeher
Copy link
Owner

Thank you. Hopefully I'll find some time to review soon. Maybe someone else will jump in sooner?

@blattm
Copy link
Contributor Author

blattm commented Jan 26, 2026

I just had a look at the conflicts and it's ... a lot. main.c on the develop branch had been edited in the meantime, including some refactoring. It looks like it could take multiple hours to fix everything and I'm not quite sure about the best strategy yet.

Maybe it makes sense to postpone the review until the conflicts are resolved? I'm not sure. But I guess I'll already start working toward resolving the conflicts, because this will only get worse the more develop changes over time.

@blattm blattm force-pushed the refactor-parser branch 2 times, most recently from 9d6fc4e to 49bf600 Compare February 8, 2026 18:55
@blattm
Copy link
Contributor Author

blattm commented Feb 8, 2026

I finally managed to resolve all of the conflicts! 🎉
To achieve this, I had to merge develop's new commits one by one. This makes the commit history a little cluttered, but I guess we are going to squash this anyway.
I'd say it's ready for review now! I hope we can get this done before develop changes again 😅

@luar123
Copy link
Contributor

luar123 commented Feb 10, 2026

I had no time to look into details, but I like the structure a lot and a quick test was running fine.

@CarlosDerSeher I think it would be great if we could move forward because all the PRs or planned PRs would benefit from not modifying the same main.c file. #174 wants to wait for this, I also wait with further work. And #195 again adds a lot of modifications, that would otherwise go the the connection_manager I guess.

@CarlosDerSeher
Copy link
Owner

Oh didn't realize this was ready :) nice

@CarlosDerSeher
Copy link
Owner

Great, and just today I merged some other stuff :( hopefully this isn't so much work to resolve

@blattm
Copy link
Contributor Author

blattm commented Feb 10, 2026

Great, and just today I merged some other stuff :( hopefully this isn't so much work to resolve

It's resolved again :)

@CarlosDerSeher CarlosDerSeher changed the base branch from develop to refactor_parser February 11, 2026 12:09
@CarlosDerSeher CarlosDerSeher merged commit e234e51 into CarlosDerSeher:refactor_parser Feb 11, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants