feat: added UsbClockSelection for STM32G431xx

[Gurkengewuerz]

implemented UsbClockSelection for the STM32G431xx. My hardware is running in this configuration since last week. https://github.com/Gurkengewuerz/mLRS-custom-builds/blob/main/customTargets/mc8051-diy-g431cb/Core/Src/main.cpp#L170-L176

Tested on STM32G431CB

[olliw42]

many thx!

I however have serious doubts.

When I create code for USB with CubeMX it does call HAL_RCC_OscConfig() with RCC_OSCILLATORTYPE_HSI48 and RCC_HSI48_ON, which eventually calls __HAL_RCC_HSI48_ENABLE() which sets bits in register RCC->CRRCR.

Your code addition calls HAL_RCCEx_PeriphCLKConfig() with RCC_PERIPHCLK_USB, RCC_USBCLKSOURCE_HSI48, which eventually calls __HAL_RCC_USB_CONFIG() which sets bits in register RCC->CCIPR.

That's totally different registers.

Furthermore, when I look up in the code you've linked to I do see that it also does call HAL_RCC_OscConfig() with RCC_OSCILLATORTYPE_HSI48 and RCC_HSI48_ON few lines up. This however is NOT been done in the code here.

I am forced to conclude that

1. the code addition you PRed is not reflecting the required changes to get it to work for main by judging from your code.
2. you have NOT tested your PR but in fact have tested a different code, in contrast to your statement

I want to repeat this: You MUST test your PR with the actual code! Not with some other code. If you haven't tested it on the actual main code, then at least say so, but don't provide misleading statements.

I hope this doesn't come across harsh, if so I'm sorry, that's not my intention. I very much appreciate your efforts and contribution, but I think it needs a bit more care and attention also to details.

Does this make sense?

[olliw42]

ok, to give you some idea what I think - with no testing whatsever - it could look like

#if defined STM32G431xx  
    // initialize HSI48, copied with adaption from SystemClock_Config()
    RCC_OscInitTypeDef RCC_OscInitStruct = {};
    RCC_OscInitStruct.OscillatorType = RCC_OSCILLATORTYPE_HSI48;
    RCC_OscInitStruct.HSI48State = RCC_HSI48_ON;
    RCC_OscInitStruct.PLL.PLLState = RCC_PLL_NONE; // important, since otherwise HAL_RCC_OscConfig() woudl set it
    if (HAL_RCC_OscConfig(&RCC_OscInitStruct) != HAL_OK) {
       //Error_Handler();
    }
#endif

    // copied from SystemClock_Config()
    RCC_PeriphCLKInitTypeDef PeriphClkInit = {};
    PeriphClkInit.PeriphClockSelection = RCC_PERIPHCLK_USB;
#if defined STM32F103xE
    PeriphClkInit.UsbClockSelection = RCC_USBCLKSOURCE_PLL_DIV1_5;
#elif defined STM32G431xx
    // CubeMX does actually not add this to SystemClock_Config(), but it is needed
    PeriphClkInit.UsbClockSelection = RCC_USBCLKSOURCE_HSI48;
#elif defined STM32F072xB
    PeriphClkInit.UsbClockSelection = RCC_USBCLKSOURCE_PLL;
#endif
    if (HAL_RCCEx_PeriphCLKConfig(&PeriphClkInit) != HAL_OK) {
        //Error_Handler();
    }

[Gurkengewuerz]

for more context commented on this on the mLRS Discord https://discord.com/channels/1005096100572700794/1005096101239603232/1156935794611716168

[Gurkengewuerz]

To keep this PR updated on GitHub: We put this PR into WIP because more testing is required for all G431 targets like the G431KB in the mLRS repository. We are in consent that "It works on my hardware" is not the solution and more validation is required. I will implement Olli changes and also send him my hardware version with the G431CB, so he can test and validate changes better.

Looking at PRs with criticism is required to keep a project clean especially on the main branch.

[Gurkengewuerz]

the main.cpp is now identical.

I added your changes as you have prescribed @olliw42 and tested it on the G431CB. let's wait for the hardware to arrive. maybe there are more changes to the code. ✌🏼

cheers

btw deleted my last post because of a wrong picture

[olliw42]

the changes appear to be not fully sufficient, but at the end of usb_deinit() we need to also add

#if defined STM32G431xx
    RCC_OscInitTypeDef RCC_OscInitStruct = {};
    RCC_OscInitStruct.OscillatorType = RCC_OSCILLATORTYPE_HSI48;
    RCC_OscInitStruct.HSI48State = RCC_HSI48_OFF;
    RCC_OscInitStruct.PLL.PLLState = RCC_PLL_NONE; // important, otherwise HAL_RCC_OscConfig() would set PLL
    if (HAL_RCC_OscConfig(&RCC_OscInitStruct) != HAL_OK) {}
#endif

otherwise jumping to teh systembootloader seems to can leave the mcu in starnge states (I got issues with an error DEV_TARGET_NOZ_HALTED)

[Gurkengewuerz]

thank you Olli for checking and reviewing. I implemented your requested changes. Testing the system bootloader is a thing I haven't done just because I am not aware of all the features. I guess having test hardware on your desk was the best option!

[olliw42]

fantastic! MANY THX.

fyi: I have updeated run_copy_st_drivers.py and CREATE_TARGET_INSTRUCTIONS.txt to help with targets suing usb. I t would be great if you would recreate your targets following the descriptions. To see that it works.

as it might be that not yet all little bugs have been weeded out, it indeed might be usefull to have tha test board here. MANY THX for it!