Refactor Parser

[blattm]

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.

[CarlosDerSeher]

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

[blattm]

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]

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]

With the new connection component, I was able to drastically simplify the parser. I know this isn't perfect yet and I'll probably spend some more time cleaning this up.

Some thoughts:

• It works perfectly fine on my board. I don't know how to measure memory usage, but I can't imagine that it was increased by much.
• This would be a good time to think about all possible errors in message parsing/handling and if we're dealing with them in a good way.
• The connection handler is now a separate part and could be modified to use a different library without the need to touch the parser again. But this would only make sense in a separate PR.
• It could be slightly more efficient to give the parser a function to fetch an arbitrary amount of bytes instead of connection_get_byte, which works byte by byte, but maybe this is just a micro optimization.

Let me know if you have feedback!

[CarlosDerSeher]

Make this an enum or maybe even a type

[CarlosDerSeher]

It works perfectly fine on my board. I don't know how to measure memory usage, but I can't imagine that it was increased by much.

There is a function heap_caps_get_largest_free_block() which can be used to profile heap usage. You'll have to find a good place to call and print this though.

It could be slightly more efficient to give the parser a function to fetch an arbitrary amount of bytes instead of connection_get_byte, which works byte by byte, but maybe this is just a micro optimization.

I agree, netconn gives you a buffer on which we operate dirtectly with its size, so maybe a function that gets the current buffer pointer and remaining size would be better.

The connection handler is now a separate part and could be modified to use a different library without the need to touch the parser again. But this would only make sense in a separate PR.

I like this, this will improve portability quite a bit.

[luar123]

@blattm what are your plans for this PR? There have been some changes so you need to adapt to those.

I am asking because I like your changes a lot but I wanted to rework the task communication/synchronization. So I could do that based on the current dev branch (then you would need to include these changes as well) or wait until your work is done. If you are planing to finish it soon I would prefer to wait.

[blattm]

Hey everyone, and happy new year!

Sorry for the long silence on my side.

I initially had fairly ambitious plans to build an extremely clean solution, restructuring almost everything touched by this PR. But that turned out to be a bit more than I can realistically handle right now.

My suggestion would be to merge the PR as it is (or with only minimal changes required for code quality) and take care of any newer commits on the dev branch. That would allow me to get back to feature work and bug fixes and possibly revisit the refactoring later, when it’s less of a burden.

I’m currently ill and won’t be able to actively work on this for the next one or two weeks. Feel free to wrap this up and merge it on your side. Otherwise, I’ll take care of it once I’m back.

Thanks again for all your work and keeping things moving :)

[luar123]

Happy new year, and get well soon!

@CarlosDerSeher do you think this is in a state where it could be merged (after solving all the conflicts from UI and time message changes)? I am not sure if I have time, but I might give it a try. If solving the conflicts is all that is needed. Did not look into details yet.

[CarlosDerSeher]

Last time I checked it looked quite mature to me. There has been one minor thing, the passing of a lot of different parameters to the functions. Those could be aggregated into a single struct. It's been a while I had a look though.l, maybe there is more ro be done.

[CarlosDerSeher]

Another thing, I don't like the name of the component snapcast_custom_parser, couldn't we call it snapcast_tcp_parser, sc_tcp_parser, snapcast_protocol_parser or something like that. I am open to suggestions

[blattm]

Alright, I'm back :)
I'll start with the renaming and structs for parameters.
What's the plan afterwards? Should I target the current state of the develop branch? Or wait for the develop->master merge to be completed?

[CarlosDerSeher]

Use develop as it most up to date

[blattm]

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 🙂

[CarlosDerSeher]

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

[blattm]

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.