Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dependency with cmsis v1 #106

Closed
michaelb77 opened this issue May 25, 2023 · 24 comments
Closed

Dependency with cmsis v1 #106

michaelb77 opened this issue May 25, 2023 · 24 comments

Comments

@michaelb77
Copy link

Good day,

I am currently incorporating the ubxlib into a project using CMake. It seems, however, that the series of files located under the "port" directory include cmsis os version 1. Since we are using cmsis os version 2, I was wondering if there is a workaround to prevent the inclusion of this version of cmsis. Are the "port" files suppose to be edited and modified?

Regards,

Michel

@RobMeades
Copy link
Contributor

Hi there, and thanks for posting. Which platform are you intending to build ubxlib for? I guess probably STM32F4, if CMSIS is being brought in; that's the only platform we have which uses it I believe.

For STM32F4 we #include "cmsis_os.h", which we expect to find in the STM32F4 SDK, but looking at the version of that we test with (version 1.25.0), the CMSIS README file says it is version 5.4.0, which is a bit confusing. I also notice that the FreeRTOSConfig.h, which will have come from the STM32F4 SDK, has comments about CMSIS version 2 in it, all of which look a little worrying:

/*------------- CMSIS-RTOS V2 specific defines -----------*/
/* When using CMSIS-RTOSv2 set configSUPPORT_STATIC_ALLOCATION to 1
 * is mandatory to avoid compile errors.
 * CMSIS-RTOS V2 implmentation requires the following defines
 *
#define configSUPPORT_STATIC_ALLOCATION          1   <-- cmsis_os threads are created using xTaskCreateStatic() API
#define configMAX_PRIORITIES                    (56) <-- Priority range in CMSIS-RTOS V2 is [0 .. 56]
#define configUSE_PORT_OPTIMISED_TASK_SELECTION 0    <-- when set to 1, configMAX_PRIORITIES can't be more than 32 which is not suitable for the new CMSIS-RTOS v2 priority range
*/

/* the CMSIS-RTOS V2 FreeRTOS wrapper is dependent on the heap implementation used
 * by the application thus the correct define need to be enabled from the list
 * below
 *
//define USE_FreeRTOS_HEAP_1
//define USE_FreeRTOS_HEAP_2
//define USE_FreeRTOS_HEAP_3
//define USE_FreeRTOS_HEAP_4
//define USE_FreeRTOS_HEAP_5

*/

Can you say more about the exact nature of the problem?

You can, of course, modify any part of ubxlib but it would definitely be better if you didn't have to. A quick check of the Keil page concerning this suggests that it might just be a load of irritating renaming, which we could accommodate easily enough, but we'd have to find out how to make the ST stuff use version 2, which isn't very clear to me.

@michaelb77
Copy link
Author

Good day Rob,

Thanks for responding so rapidly. Actually, there is quite a few files under the STM32 port which include the cmsis_os.h file: u_port.c, u_port_os.c, ... First, I simply tried to add the latest version of CMSIS without commenting the previous one, in hope that they would be mutually exclusive. However, that prevented the inclusion of the second header. Basically, some data structures have been redefined and quite a few function calls are different in the latest version which triggers a series of compile errors: osThreadDef_t, osThreadCreate(), ... That comes down to several modification from one version to the next: renaming, different type of function calls, etc. Now some files seem to include CMSIS directly, amongst which:

  • u_port.c
  • u_port_i2c.c
  • u_port_os.c
  • u_port_uart.c

The atlernative of directly editing those files whould be to have a sort of interface, using function pointers to be configured initially or else, functions that must be implemented by the user of the library. That would remove CMSIS from being directly called and gives more flexiblity. In some of our tests, we use a simple loop to validate the functioning of a module. Under those circumstances, the inclusion of CMSIS v1 is not that problematic. However, we deploy our products with CMSIS v2, which makes those modules unsuable without modification. We would prefer to download the ublx library as-is without modifying it. Let me know if you see an alternative beside directly editing the files or if you want me to try something on my side.

Thanks again for your help.

Regards,

Michel

@RobMeades
Copy link
Contributor

Thanks for the detail.

Question: I guess you are using the STM32F4 SDK: how do you make it use CMSIS V2 instead of CMSIS V1? I had a quick look in the Makefile we use for testing but we don't seem to set anything obvious.

If it could make our system attempt to build/test CMSIS v2 instead of v1, assuming it is already present in the STM32F4 FW version 1.26.0, then I could have a quick hack at seeing if we could make it a compile-time option.

@michaelb77
Copy link
Author

Good day Rob,

I am using a generated STM32F4 library which sits on our server. We simply include the proper MCU version using CMake. Now, the previous inclusion of CMSIS v1 I was refering to was done using CubeMX to validate the dependency. Using a test project I simply include the library (ublx) as a dependency and I try to compile it. At this time, I am not using the example provided by ublox with the runner since I am not using the STM IDE either.

The Makefile includes FreeRTOS, while the files mentionned previously are calling the functions through CMSIS. Now, I am not an expert with FreeRTOS, but maybe the module itself is the same, while the interface (CMSIS) has changed.

Regards,

Michel

@RobMeades
Copy link
Contributor

OK, I've found out how to make it bring in CMSIS V2. I'll have a poke around to see how hard it would be to make it a compile-time option.

@RobMeades
Copy link
Contributor

Update: I've managed to make a version which compiles CMSIS V2. It is not yet running (falls over in an assert inside FreeRTOS queue.c at startup) but, before I go any further, we should verify that this at least builds in your environment now.

You can find the code here: preview_stm32f4_cmsis2_rmea. To build for CMSIS V2, pass the flag CMSIS_V2 into your build of ubxlib.

How long could you wait for this to be completed?

@michaelb77
Copy link
Author

Good day Rob,

That's awsome news! I will be back at the office on Monday to try it. My company expect the product to be ready in a about a month from now. I will keep you posted.

Thanks again for your help, really appreciate that!

Michel

@RobMeades
Copy link
Contributor

My company expect the product to be ready in a about a month

You don't hang about! I'll see if I can make it run as well...

@michaelb77
Copy link
Author

Good day Rob,

Thank you very much. The speed at which you came up with this patch is amazing!

I am working on it at this time and I will let you know how things are going.

Regards,

Michel

@RobMeades
Copy link
Contributor

Good stuff: like I say, it doesn't actually run yet, I think they've changed all of the xxxCreate() functions to be xxxNew() functions and, though the code compiles, I only changed the task creation function from an "xxxCreate()" to an "xxxNew()" and it might be that is why the semaphore stuff is asserting.

If the code compiles/links for you then we're heading in the right direction; if it doesn't then let me know what fails and that might give us some pointers.

@michaelb77
Copy link
Author

Good day Rob,

Indeed, everything does compile! Thanks! I guess next step is to fix the assert and see if the code behaves as it should. As of now, I am waiting for the hardware to arrive at the office. Let me know if you manage to progress on your side.

Regards,

Michel

@RobMeades
Copy link
Contributor

Phew, that's good news, I'll let you know when I've got further and will push an updated preview branch here.

@RobMeades
Copy link
Contributor

Branch is now updated (unit of stack size (words v bytes) and task termination misunderstandings): this version has passed all of our internal regression testing. I run it through a few more times tonight, just to be completely sure there are no statistical failures, then I will merge it to our internal master, push that out here and delete the preview branch, hopefully tomorrow.

When do you think your HW will be to hand?

@michaelb77
Copy link
Author

Good day Rob,

Wow, that's really nice! As far as I know, the hardware should be available in 1-2 weeks maximum. That will give me time to complete our driver and link it with your library. I will do some unit testing here before the hardware integration. I will get back to you with an update for sure.

Again, thank you very much.

Regards,

Michel

@RobMeades
Copy link
Contributor

The change is now pushed to master here, see c759fdb. I'll delete the preview branch now but let's keep this issue open until you've confirmed that it works on your target HW.

@michaelb77
Copy link
Author

michaelb77 commented May 31, 2023

Good day Rob,

While integrating other modules for the project, I ran into issues with some files included in the library which prevent the code to compile:

u_port_private.c

  • lines 121, 122, 123, 135, 136, 137: GPIOs defined while the F4 does not have them by default.

u_port.c

  • Including stm32f437xx.h clashes with local F4 default defines.

u_port_gpio.c

  • lines 581, 605-617: Function LL_GPIO_Init() and type LL_GPIO_InitTypeDef not recognized.

u_port_spi.c

  • lines 96,97: SPIs defined while the F4 does not have them by default.
  • lines 148.., 185...: Macros related to SPIs not defined.
  • lines 201: Function getPeripheralClock() is unsuable because of type LL_RCC_ClocksTypeDef and function
    LL_RCC_GetSystemClocksFreq(), which are not in present in the STM API.
  • lines 266: Function initGpio() is unsuable because of type LL_GPIO_InitTypeDef and function LL_GPIO_Init(), which are not in present in the STM API.

u_port_uart.c

  • lines 181: UART peripherals not supported by the F4.
  • lines 387: UART peripherals not supported by the F4.
  • lines 581, 946, 947, 1012-1040: Type LL_USART_InitTypeDef and functions LL_GPIO_InitTypeDef() and LL_USART_Init(), which are not in present in the STM API.

Let me know if you want me to test anything further.

Regards,

Michel

@RobMeades
Copy link
Contributor

RobMeades commented May 31, 2023

Hi again: yes, there will be this problem - we build for the maximal STM32F4, which is of course fine for all flavours of F4, 'cos the less-capable ones just won't use the things that they don't have, if you see what I mean, but ST provide several dozen flavours of the header files tailored to each individual flavour of STM32F4, which creates a complete nightmare of #defines that it is extremely difficult to test!

Which type of STM32F4 are you building for?

EDIT: I'm surprised about LL_GPIO_InitTypeDef/LL_GPIO_Init() and LL_USART_InitTypeDef/LL_USART_Init() though - these are fundamental parts of the STM32F4 FW: can you see why they are not present?

@michaelb77
Copy link
Author

Good day Rob,

Currently, I am using the STM32F446RET as a development platform and our projet will be on a STM32F412VGT6. So both MCU have to be supported.

Regards,

Michel

@RobMeades
Copy link
Contributor

They are supported: we compile with STM32F437VG because it is the largest beast of the pack, therefore all sub-types are supported, it is ST's maze of header files that you will need to navigate.

LL_GPIO_InitTypeDef/LL_GPIO_Init() and LL_USART_InitTypeDef/LL_USART_Init() are fundamental parts of the STM32F4 FW though: can you see why they are not present?

@michaelb77
Copy link
Author

Good day Rob,

Ok, let me see the configuration of the project and I will get back to you.

Regards,

Michel

@michaelb77
Copy link
Author

Good day Rob,

We were using the higher level layer to access the hardware (HAL) instead of the lower level (LL). I switched and it does compile.

I will get back to you once I get the hardware.

Regards,

Michel

@RobMeades
Copy link
Contributor

Phew! That's good news, thanks for the update.

@RobMeades
Copy link
Contributor

Hi @michaelb77: I'm going to close this as I guess you got what you needed in respect of CMSIS dependencies. If you disagree, please feel free to re-open this issue.

@michaelb77
Copy link
Author

Hi Rob,

Got it working with hardware. Thanks for your support.

Regards,

Michel

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

No branches or pull requests

2 participants