Re: HALs in Zephyr (was Re: STM32Cube SDK in Zephyr)


Fabien Parent <fparent@...>
 

Hi Amit,

On Tue, Nov 8, 2016 at 8:51 AM, Amit Kucheria
<amit.kucheria(a)verdurent.com> wrote:
Hi Erwan,

On Thu, Nov 3, 2016 at 8:07 PM, Erwan Gouriou <erwan.gouriou(a)linaro.org> wrote:
Hi all,


There are growing number of STM32 based boards being ported in Zephyr
by the community. As part of ST, I can say that we are pleased to contribute
this way to Zephyr impact in IoT world.

In order to ease porting of STM32 devices, we'd like to introduce STM32Cube
SDK into Zephyr. Aim is to make porting fast and easy thanks to ST CSMIS
files, reduce code duplication and provide mature software with STM32Cube
adaptation layers (HAL and LL).
I'm all for allowing Zephyr to quickly add support for as much
hardware as possible and if HALs are the way to do it, then so be it.

However, I'd like to hear from Zephyr maintainers on whether this is
just a short-term strategy to get broad hardware support or the long
term goal because HALs do make for hard-to-read code[1] and each
vendor's HAL is different leading to further maintenance issues.

I ask this coming from a Linux development mindset where HALs are
actively discouraged.
I'm also interested in this topic. I posted several weeks ago on
gerrit some patches to add support for the STM32L4 family. The
reviewers wanted me to import the ST headers (they were not there at
the time) to reuse the peripheral's base register addresses, I do not
fully agree with it but this is just a few defines from ST headers
that follow the same coding convention as Zephyr so that's fine. Now
the reviewers also want me to start using the structure that defines
the peripherals registers from the ST headers, but here that's where
it starts to bothering me: this code does *NOT* follow the Zephyr
coding style at all.

Example:

typedef struct
{
__IO uint32_t MODER; /*!< GPIO port mode register,
Address offset: 0x00 */
__IO uint32_t OTYPER; /*!< GPIO port output type register,
Address offset: 0x04 */
__IO uint32_t OSPEEDR; /*!< GPIO port output speed register,
Address offset: 0x08 */
__IO uint32_t PUPDR; /*!< GPIO port pull-up/pull-down
register, Address offset: 0x0C */
__IO uint32_t IDR; /*!< GPIO port input data register,
Address offset: 0x10 */
__IO uint32_t ODR; /*!< GPIO port output data register,
Address offset: 0x14 */
__IO uint32_t BSRR; /*!< GPIO port bit set/reset register,
Address offset: 0x18 */
__IO uint32_t LCKR; /*!< GPIO port configuration lock
register, Address offset: 0x1C */
__IO uint32_t AFR[2]; /*!< GPIO alternate function registers,
Address offset: 0x20-0x24 */
__IO uint32_t BRR; /*!< GPIO Bit Reset register,
Address offset: 0x28 */
__IO uint32_t ASCR; /*!< GPIO analog switch control register,
Address offset: 0x2C */

} GPIO_TypeDef;

I feel that if I have something that works, that does not duplicate
existing code, and that follow the existing coding style, my code
should be merged as-is. But right now we are asking me the opposite:
to start using symbols that do not follow the coding style.

If that's what maintainers want, I will follow even if I don't like
it, but at least I would love to see an official guideline from the
maintainers of what is the preferred approach here.


About STM32Cube CMSIS files:
Proposition for now is to move progressively towards the support of
STM32Cube
on available STM32 based boards and that new boards include STM32Cube CMSIS
files from start.
Shouldn't you import all possible HALs _today_ in order to enforce
that? Otherwise it'll lead to unnecessary churn when people are doing
platform/SoC ports.

Regards,
Amit
[1] What is it with vendor code and camel-case and excessive use of
typedefs? :-)

Join devel@lists.zephyrproject.org to automatically receive all group messages.