FreeRTOS / FreeRTOS-Plus-TCP

FreeRTOS-Plus-TCP library repository. +TCP files only. Submoduled into https://github.com/FreeRTOS/FreeRTOS and various other repos.
MIT License
125 stars 149 forks source link

Fix header includes #1120

Closed HTRamsey closed 3 months ago

HTRamsey commented 3 months ago

Hopefully will allow #1117 to pass Ensures correct ordering of includes for everything. Might be a bit pedantic, but having the core/optional include sections promotes the idea of add-on features rather than having everything intertwined. Similar to how cyclone is organized which I'm a big fan of. Also, I removed the ipconfig checks for the headers because in my opinion it's cleaner to just guard the entire implementation with the ipconfig checks rather than the headers, which will have the same effect during builds but with less clutter. I'd suggest leaving them if the source files were organized in a module manner rather than a single root folder.

n9wxu commented 3 months ago

FreeRTOS_IP_Common.h is a bad idea. There is really nothing common about that header file and adding additional headers just creates dependencies where there should not be any.

I am reverting this PR. The common header should go away entirely with the other contents moved into new headers appropriate to the purpose. The OP compared with CycloneTCP as a a good example. I agree, the structure of CycloneTCP should be followed here with many small headers specific to the needed functionality.

HTRamsey commented 3 months ago

I agree that each header should only include what it needs, which the vast majority of them will at least need

#include "FreeRTOS.h"
#include "task.h"

#include "FreeRTOSIPConfig.h"
#include "FreeRTOSIPConfigDefaults.h"

and most of the typedefs in the headers should be avoided to allow forward declarations. Also many of the source files currently basically include everything. Right now most of the headers are missing the includes they need, which also caused issues with the freertos config values and freertos ipconfig values being available in the headers if they weren't included in certain orders. Unfortunately a restructuring like cyclone is a good deal of work.

cobusve commented 3 months ago

There is probably a good case to consolidate the Config and defaults, if config has to ALWAYS be followed by defaults then it can be included in the config header file making the semantics of the dependency clear and resolving the risk of include order, but I agree we cannot make an aggregate header that includes all headers.

We are in the process of trying to restructure and we must make sure every step we take takes us closer to the target not the opposite direction.

At this point if anybody finds a header that uses something declared in a different header please fix that instance by including the header which it clearly depends on instead, that takes us closer to the goal.

We should also remove headers that are being included but are not being used wherever we see this.

HTRamsey commented 3 months ago

@cobusve Since the config is a user provided file I guess we would have to just rely on the check here. While I clearly did it incorrectly, the purpose of this PR was to address the issue here where the problem was that FreeRTOS.h & task.h were needed to be included before FreeRTOSIPConfig.h & FreeRTOSIPConfigDefaults.h for each header, which wasn't the case for FreeRTOS_DNS.h at least. An alternative is to just not do any compile time config checks. Then you wouldn't need FreeRTOS.h & task.h for the config files. Or add more ifndef checks in the headers to make sure the includes are in the correct order.