Major bug: S2LP Driver code does not compile

[samuelsd1]

Driver code does not compile due to incorrect code.

Setup

• Board: NUCLEO-L412KB
• BSP: S2868A2 (Reproduces anywhere, software related)
• IDE: STM32CubeIDE 1.8

Context

I am comparing the code of this repository - to the S2LP driver code I acquired using STM32CubeIDE software package manager (following STMicroelectronics YouTube tutorial)

I will refer to the code acquired following this tutorial as the original code, since it is a release version, therefore it is safe to assume more people have the same driver code.

Bug details

Code that causes the problem:

• Calls S2LPSpiReadRegisters instead of S2LP_ReadRegister (The first one is not defined therefore incorrect)
  • Source files: s2lp_general.c ,s2lp_types.c
• Multiple definition of ModeExtRef enum:
  • Original definition in s2lp.h (exists in current release of x-cube-subg2)
  • Duplicate occurrence in s2lp_general.h

• Calls to undefined function/macro s_assert_param (not defined at all in the repository).

  • Source files: s2lp_general.c

  Additional code problems

• Missing import of s2lp_general.h in s2lp.h.
  • This masks all the problems in s2lp_general files. but this does not prevent the compilation errors.
  • The s2lp_general files exist in the repo and their purpose seems correct.
  • In my opinion they should not be deleted, but rather fixed
• Duplicate definition of S2LPGeneralLibraryVersion()
  • Source files: s2lp.h, s2lp_general.h
  • In my opinion it should stay in s2lp.h
• Duplicate functions in s2lp_general files, which already defined in s2lp.h and implemented in s2lp.c:
  • S2LPGeneralGetDevicePN, already exists as S2LP_GetDevicePN
  • S2LPGeneralGetVersion, already exists as S2LP_GetVersion
  • S2LPGeneralSetExtRef, already exists as S2LP_SetExtRef
  • S2LPGeneralGetExtRef, already exists as S2LP_GetExtRef
  • S2LPRadioSetExternalSmpsMode, already exists as S2LP_SetExternalSmpsMode
  • S2LPRefreshStatus, already exists (and widely used) as S2LP_RefreshStatus Since the functions already exist, it would be incorrect to delete them (as this would break compatibility to the previous release version). Therefore - a good solution would be to keep the existing functions, move them into s2lp_general files, while removing the duplicate functions defined in the s2lp_general files. This way the functionality and compatibility will be preserved. In addition, this solution seems to align with the purpose of s2lp_general files.
• Duplicate definitions of the g_xStatus variable
  • Original one in s2lp.c
  • Duplicate one in s2lp_types.c This does not cause compilation error, but it is incorrect. A fix to this would be to choose one between the two.
  • _In my opinion, s2lp_types should not contain the definition of g_xStatus since it is not a type. It is rather a global instance variable. Therefore I believe it should be declared in s2lp.h and defined in s2lp.c (similar to the IO_func variable in s2lp.c)_

How to reproduce the bug

Copy the source files located in Drivers/BSP/Components/S2LP to your project, then compile.

Additional context

I am using the driver code to write S2-LP high-level interface, which is platform independent. Therefore, the code is more sensitive to platform dependent definitions. Example of such platform dependent code:

• The driver code contains the function s_assert_failed, and calls to undefined s_assert_param macro. The current behavior of a failed assert is a printf call, and infinite error loop. Driver code should not behave this way. Parameter validations are important and should be performed, but the result of bad parameter should be returning an error code. Therefore, in my patch I temporarily defined empty macro for s_assert_param, but I believe both s_assert_param and s_assert_failed should be removed from the driver code.

Solution

I have patched the mentioned problems, compiled and tested my patch by running simple P2P app. Soon I will open a pull request containing my tested patch.

[mgrella]

Thank you for your feedback. I'll look in it more in deep next week but as far as I can see you are referring to files (s2lp_general.c and s2lp_types.c) that actually are in the driver code but are not used in the projects (you can check opening one of them), that's probably why they were not updated in the refactoring of original S2-LP Driver.

Next version of the package will include original S2-LP Driver from S2LP-DK, and wrap it in a different way, avoiding these kind of problems.

Coming back to your problem, how did you produce the project? I think that If you remove from it those unused files, it should compile. By the way, STM32CubeMX tool can be used to migrate to other platforms and generate code, and x-cube-subg2 is integrated into it.

BR, Marco

[samuelsd1]

@mgrella Thank you for your response :)

I commented on the pull request about the technical details.

I have some questions: In the pull request (and also here), you mentioned next version will include the driver with a different approach.

1. I would like to hear more details about the new release approach
2. Isn't the driver developed directly on this repository? (I assumed it is, since you open sourced the driver). Is there another development repository / version?

With much thanks, David

[mgrella]

Hello David,

let me start from your question number two :) This repo is intended to delivery the x-cube-subg2 package that delivers a set of Examples (P2P demo) and Applications (projects that uses not only the Drivers but also a Middleware, in this case Contiki-NG) on top of S2-LP radio.

As you know, the radio driver itself can be found in Drivers/BSP/Components/S2LP. But this is not the direct focus of this repository, and it is a "readapted" version of the original S2LP Driver that you can find in the STSW-S2LP-DK: https://www.st.com/en/embedded-software/stsw-s2lp-dk.html This package contains the "original" driver, currently not yet distributed via GitHub.

I wrote "readapted" because the x-cube-subg2 package is also a pack for STM32CubeMX: https://www.st.com/en/development-tools/stm32cubemx.html a tool that, among other things, allows you to reconfigure a given sw solution for different families of MCU. This task requests a different approach for the bus communication, that's why the original S2-LP driver, here, is wrapped by means of specific Board Support Packages (Drivers/BSP/S2868A1, ...) and provides a different kind of interface (s2lp.c/h).

In the context of this "adaptation", also a refactoring of the functions was performed, and this causes the inconsistency that you correctly point out, since some files (unused by our projects) have not been refactored.

Coming back to your question number 1: the new release will include the original S2LP_Library coming from STSW-S2LP-DK, as a core part of x-cube-subg2 S2-LP Component Driver, with no refactoring. Backward compatibility with current "refactored" API will be granted by means of proper mapping h file. In my opinion, this will makes things more clear and will also be more future-proof in case S2-LP core driver will be hosted on a separate repository.

BR, Marco

[mgrella]

Closing this issue, S2-LP Component Driver now contains official S2-LP Library from S2-LP DK. We'll check the other feedbacks for further updates in following months