Clean OSAL API

[ismilak]

Cleaning OSAL API and make it more portable for other platforms. Fixing function parameter signatures and add additional error handling where the OSAL APIs are used. Removing linux header references, and using only OSAL to make the lib core independent on the platform.

[silabs-ThibautC]

Please share the test strategy/process being followed to test FreeRTOS specific changes and also to test existing Linux implementation(just to ensure nothing is broken for Linux)

@manojnacsl May I return the question? Do you have test infrastructure/suites that we could rely on?

[ismilak]

Please share the test strategy/process being followed to test FreeRTOS specific changes and also to test existing Linux implementation(just to ensure nothing is broken for Linux)

Hi Everyone,

It has been tested manually.

• Build test: make and ./build.sh
• Smoke test: I went through step by step on the tutorial doc and the agent was able to connect to and communicate with the FND. I have a Cisco FND VM on a linux host, and I built and executed the agent on Raspberry Pi4 and on the linux host. (Ubuntu 22.04)

I note that, freertos.target has not been tested in this phase.

[manojnacsl]

Please share the test strategy/process being followed to test FreeRTOS specific changes and also to test existing Linux implementation(just to ensure nothing is broken for Linux)

@manojnacsl May I return the question? Do you have test infrastructure/suites that we could rely on?

@silabs-ThibautC - Currently we do not have a separate test suite as such. But the usual test practice we have been following for Linux target is to have the Linux agent connect to FND instance to test - registration, periodic metric refresh, and GET operations of the TLVs.

[manojnacsl]

Please share the test strategy/process being followed to test FreeRTOS specific changes and also to test existing Linux implementation(just to ensure nothing is broken for Linux)

Hi Everyone,

It has been tested manually.

* Build test: make and ./build.sh

* Smoke test: I went through step by step on the tutorial doc and the agent was able to connect to and communicate with the FND. I have a Cisco FND VM on a linux host, and I built and executed the agent on Raspberry Pi4 and  on the linux host. (Ubuntu 22.04)

I note that, freertos.target has not been tested in this phase.

@ismilak : This approach sounds good. To add to this further, you could do feature based tests Eg:

1. A working FND instance on-boarded with the Agent.

2. Build test:

   • Build the Agent for the specified target (we have been building for Linux target so far).
   • Check if it builds fine without errors and warnings.
   • It is a good practice to check if all supported targets also build fine, just to ensure no other target builds are broken.

3. Start the agent with required command line parameters (refer developer's guide)

   • Check if the Agent service starts fine and runs stable without crashes.
   • Check if the Agent registers with FND (this tests Registration feature and the Registration trickle timer functionality)
   • As part of the Agent Registration, FND does GET operation on a few TLVs - this tests GET operation of some TVLs. (enabling debug logs provides additional console logs to check)
   • Periodic Metric refresh will be triggered based on the Report trickle timer - check if this feature reported the configured TLVs to FND.

Please refer to the Developer's guide here: csmp-agent-lib/docs/CSMP Developer Tutorial - 0v11.pdf

[ismilak]

Hi @manojnacsl and @paduffy !

I solved all the findings from the current and previous PR reviews:

• common osal API return code definitions
• debug prints for API return code checking
• osal source and header file refactor, common header and osal.c/h file hierarchy modified based on @manojnacsl proposal

Could you take a look at this again?

THX!

[ismilak]

• Run a spellchecker
• Check from double withespaces

I executed spellchecker, and I standardised the comments, removed unnecessary white spaces etc. I removed the doxy comments from osal.c, I note that I just copied comments from it, that was the original location of code documentation. If it's required, I can copy back the checked and fixed doxy comments into the source file, but I think we should avoid the duplications. It makes the c file cleaner also.

[ismilak]

Additional changes:

• Rename file descriptor context to socket descriptor in the API, since we use it with sockets only
• Standardise type definitions and API function names

[silabs-ThibautC]

@ismilak @manojnacsl I apologize because I'm re-opening a topic that I think you already closed but I'm really not a big fan of the file organization you agreed upon.

• there's a file name duplication (osal.c, osal.h) which can be confusing. Today their are only two. A pair would be added for each new platform supported osal.h looks like a public API but it is actually a platform-dependent header.
• osal_common.h does not look like a public API header but it actually is.

Edit: Agreed that we need to keep the platform-specific header. Forget my two previous proposals.

osal/ ├── osal.h ├── freertos/ │ ├── osal_freertos.c │ └── osal_platform_types.h │ └ Only contains type definitions, could be renamed to make it clear. ├── linux/ │ ├── osal_linux.c │ └── osal_platform_types.h └── silabs/ ├── osal_silabs.c └── osal_platform_types.h

---

Invalid proposals

osal/ ├── osal.h ├── freertos/ | └ public API header, can be included anywhere else in the project │ ├── osal_freertos.c │ └── osal_freertos.h │ └ I'm not sure we need that file see my comment above. It should only be included in osal_freertos.c ├── linux/ │ ├── osal_linux.c │ └── osal_linux.h └── silabs/ ├── osal_silabs.c └── osal_silabs.h

If we agree that we could merge osal_platform.h into osal_platform.c it could even be simplified: osal/ ├── osal.h ├── osal_freertos.c ├── osal_linux.c ├── osal_silabs.c

[ismilak]

@ismilak @manojnacsl I apologize because I'm re-opening a topic that I think you already closed but I'm really not a big fan of the file organization you agreed upon.

• there's a file name duplication (osal.c, osal.h) which can be confusing. Today their are only two. A pair would be added for each new platform supported osal.h looks like a public API but it is actually a platform-dependent header.
• osal_common.h does not look like a public API header but it actually is.

Edit: Agreed that we need to keep the platform-specific header. Forget my two previous proposals.

osal/ ├── osal.h ├── freertos/ │ ├── osal_freertos.c │ └── osal_platform_types.h │ └ Only contains type definitions, could be renamed to make it clear. ├── linux/ │ ├── osal_linux.c │ └── osal_platform_types.h └── silabs/ ├── osal_silabs.c └── osal_platform_types.h

Invalid proposals

osal/ ├── osal.h ├── freertos/ | └ public API header, can be included anywhere else in the project │ ├── osal_freertos.c │ └── osal_freertos.h │ └ I'm not sure we need that file see my comment above. It should only be included in osal_freertos.c ├── linux/ │ ├── osal_linux.c │ └── osal_linux.h └── silabs/ ├── osal_silabs.c └── osal_silabs.h

If we agree that we could merge osal_platform.h into osal_platform.c it could even be simplified: osal/ ├── osal.h ├── osal_freertos.c ├── osal_linux.c ├── osal_silabs.c

I did the same proposal (except file names) ~1.5 month ago, what was dropped then Guys: https://github.com/CiscoDevNet/csmp-agent-lib/pull/12#issuecomment-2032051462

I don't really understand why do we need to include the platform names in the osal_xxxxx.c file names, because it's specified in the directory name, at this point in the make file you can specialise the linking for the particular platform, and disagree with the osal_platform_types.h file name too, why do we need to include "platform " in it?

Back to my proposal, I think we should use the simplest possible file names and "architecture". I note that @manojnacsl recommendation was the same as mine except the file names, so finally it seems we defined the same architecture in a long story :)

[silabs-ThibautC]

why do we need to include "platform " in it? Because it only contains type definitions that are platform-specific. All the other types should remain in osal.h

why do we need to include the platform names the simplest possible file names Using the same filename multiple times is error-prone, I think we can all agree on that.

Like said, osal_common.h does not look like a public header. It looks like a header common to osal C files that should not be included elsewhere. I'd naturally include osal.h, not osal_common.h in my project.

It does not contain anything but type definitions. See it as the equivalent of stdint.h.

[ismilak]

why do we need to include "platform " in it? Because it only contains type definitions that are platform-specific. All the other types should remain in osal.h

why do we need to include the platform names the simplest possible file names Using the same filename multiple times is error-prone, I think we can all agree on that.

Like said, osal_common.h does not look like a public header. It looks like a header common to osal C files that should not be included elsewhere. I'd naturally include osal.h, not osal_common.h in my project.

It does not contain anything but type definitions. See it as the equivalent of stdint.h.

Because it only contains type definitions that are platform-specific. All the other types should remain in osal.h -> Entire osal is platform specific, since it includes network programming interface, OS API and utils, like timer, etc. (IMHO). I would say, "osal_types.h" is crystal clear for anybody, see my 1.5 months old proposal ...

Using the same filename multiple times is error-prone, I think we can all agree on that. -> just one of the several counter-examples is port.c in FreeRTOS. I note that if you try to build an efr32 project with linux version of osal.c, you will see the error at the build time immediately, so I don't think it's a critical error-prone implementation as it is now.

See it as the equivalent of stdint.h. As I mentioned, 1.5 months ago I recommended the same solution: osal_types.h (https://github.com/CiscoDevNet/csmp-agent-lib/pull/12#issuecomment-2032051462)

Just to clarify:

• I agree with having a types.h, the name of this header is not a top-prio topic I think (again, I have proposed the same earlier)
• I agree with using osal.h name instead of osal_common.h in the library sources as the common header
• I disagree with _osal_platformtypes.h name, I think _osaltypes.h it's quite enough
• I disagree with osal_xxxx.c source file names for xxxx platforms, since the same file name is arranged to platform specific directories already, so I think it's overhead, just we can use osal.c instead (my example is FreeRTOS port.c)

[silabs-ThibautC]

Entire osal is platform specific That is true today, it might not be always the case. For instance there's a scenario where we move all the RTOS functions in a single C file osal_cmsis.c for instance. That file could be re-used by several platforms. And of course, osal.h is not.

That said, as long as we rename osalcommon.h and osal.h, respectively osal.h and osal(something-or-nothing_)types.h I'm fine.

[silabs-ThibautC]

As discussed in the meeting today (05/15): osal/ ├── osal.h ├── freertos/ │ ├── osal_freertos.c │ └── osal_platform_types.h │ └ Only contains type definitions, could be renamed to make it clear. ├── linux/ │ ├── osal_linux.c │ └── osal_platform_types.h └── silabs/ ├── osal_silabs.c └── osal_platform_types.h

[ismilak]

As discussed in the meeting today (05/15): osal/ ├── osal.h ├── freertos/ │ ├── osal_freertos.c │ └── osal_platform_types.h │ └ Only contains type definitions, could be renamed to make it clear. ├── linux/ │ ├── osal_linux.c │ └── osal_platform_types.h └── silabs/ ├── osal_silabs.c └── osal_platform_types.h

Done! Please check the content, if there isn't any new topic I am ready to merge.

[ismilak]

Test with Cisco FND was successful:

• No build issues, no warnings on Ubuntu 22.04
• Sample app connected to the FND, registration request was succesful
• It sends the metrics to the FND

[manojnacsl]

Hi @manojnacsl and @paduffy !

I solved all the findings from the current and previous PR reviews:

* common osal API return code definitions

* debug prints for API return code checking

* osal source and header file refactor, common header and osal.c/h file hierarchy modified based on @manojnacsl proposal

Could you take a look at this again?

THX!

@ismilak The latest code pushed looks good and inline to our syncup discussion. Approving.