request: Implement getters for internal HAL handle structures or make it public

[stas2z]

For some hardware advanced features (like DMA) it's impossible to combine core code and pure HAL/LL calls cuz internal handles are hidden inside core code and used only internally.
For example, for my current project i need to redefine serial rx handler to do some logic without polling, so i have to edit core to access serial_t.
Or, for example, for SPI DMA implementing i need to fetch internal spi init handle to link it with DMA, ive just moved spi handle to public section, but it's a rough way and also requires modifying core code and make core updating task more complex.

What do you think?

[fpistm]

@stas2z it is already possible to use all HAL module without any Arduino api function by enabled the HAL module Only definition:
https://github.com/stm32duino/wiki/wiki/HAL-configuration#hal-module-only

[stas2z]

@stas2z it is already possible to use all HAL module without any Arduino api function by enabled the HAL module Only definition:
https://github.com/stm32duino/wiki/wiki/HAL-configuration#hal-module-only

Sure i know about using HAL without arduino api, but it's not a case im talking about here.
Here im talking about using HAL WITH arduino api.
Most hardware init inside the core using HAL or LL defined structures like SPI_HandleTypeDef for SPI. To init dma WHILE using arduino SPI lib you need at least to link DMA with your SPI instance

DMA_HandleTypeDef hdma_spi1_tx;
void initDMA(void) {

  /* DMA controller clock enable */
  __HAL_RCC_DMA2_CLK_ENABLE();

  /* DMA interrupt init */
  /* DMA2_Stream3_IRQn interrupt configuration */
  HAL_NVIC_SetPriority(DMA2_Stream3_IRQn, 0, 0);
  HAL_NVIC_EnableIRQ(DMA2_Stream3_IRQn);

  hdma_spi1_tx.Instance = DMA2_Stream3;
  hdma_spi1_tx.Init.Channel = DMA_CHANNEL_3;
  hdma_spi1_tx.Init.Direction = DMA_MEMORY_TO_PERIPH;
  hdma_spi1_tx.Init.PeriphInc = DMA_PINC_DISABLE;
  hdma_spi1_tx.Init.MemInc = DMA_MINC_ENABLE;
  hdma_spi1_tx.Init.PeriphDataAlignment = DMA_PDATAALIGN_HALFWORD;
  hdma_spi1_tx.Init.MemDataAlignment = DMA_MDATAALIGN_HALFWORD;
  hdma_spi1_tx.Init.Mode = DMA_NORMAL;
  hdma_spi1_tx.Init.Priority = DMA_PRIORITY_VERY_HIGH;
  hdma_spi1_tx.Init.FIFOMode = DMA_FIFOMODE_DISABLE;
  if (HAL_DMA_Init(&hdma_spi1_tx) != HAL_OK) {
    Error_Handler();
  }

  // Link SPI and DMA
  (&SPI._spi.handle)->hdmatx = &(hdma_spi1_tx);
  (hdma_spi1_tx).Parent = (&SPI._spi.handle);
}

here the example how i use it, to allow such link i need an access to internal _spi handle.

[fpistm]

Well, this need to be studied deeply.
Doing this would bring some regressions and/or lot of support, I guess.
Moreover, I don't know if this is enough to properly use DMA...

[stas2z]

Its working as its a paste from working project, im using dma to write framebuffer to spi lcd
Its a rough hack so i suggest to implement getter instead of making struct public

[uzi18]

maybe it can be available as special define with comment 'at own risk'

[stas2z]

maybe it can be available as special define with comment 'at own risk'

yes, it will be reasonable solution

[aheid]

maybe it can be available as special define with comment 'at own risk'

Yes, look at say Boost.ASIO, where you can get the underlying platform handle using native_handle() function1, which returns a native_handle_type which is typedef'ed to whatever. Just document that using such a function is done at programmers own risk and no support should be expected.

[ag88]

i think this including the referenced issue basically is similar
#1285
i'm of an opinion of adding the dmaSend and dmaRecieve extensions from libmaple core.
(and we'd need to think a bit further e.g. do we return an uint8_t error code (0 = success) as like the f1 core or
void (no return) as like steve's libmaple. i'd think returning an error code makes the api more versatile and
helps surface 'user' hardware configuration errors than simply quietly not returning errors.
in effect adding the api amount to changing the spi,h and spi.cpp global apis, defining a 'new' standard.
there are also specific design & docmentation issues (e.g. what error codes?) we'd need to confront with that as well)

However, in addition it turns out i realized where is part of that difficulty, take for instance
stm32f103 vs stm32f4xx, the dma pheriperials on stm32f4 is more advanced than stm32f103.
hence, even if say we 'call' HAL to do that, the codes may be rather different between stm32f103 vs stm32f4 perhaps
and we'd even need to distinguish 'platforms' that doesn't do dma and address all the soc differences,
so we may end up with a 'mess' of if defs if a proper 'optimised design' is not explored up front. i.e. 'rushing' into dma may result in 'api lock in' i.e. subsequently when we try to change the api we'd break existing apps that depend on it
my thoughts are that we can 'go slow' and even 'trial' forks without merging into the 'trunk' too soon
and only for specific chips / socs at a start. these are 'difficult' in part due to the hardware variations
and we are literally making a 'HAL' in spi.h, spi.cpp that abstract away the various hardware details
this doesn't look like a 'small' thing except in a situation where one is implementing a specific use case on a specific soc,
anything bigger than that can be quite daunting a task

[stas2z]

@ag88
This core covers most stm32 hardware, not only f103 as rogers/libmaple core.
Making universal dma init for spi transfers is very very very complex task, it much more complex imho than universal timers library we already got.
Thats why im asking for accessing handles here, it's not only for dma, it's for any task beyond the core limits

[ag88]

@stas2z i think those are good ideas, but in the same way i'd guess it'd take an 'api design' somewhat.
e.g. one of the idioms in the 'duino' world is using module.begin() calls, perhaps it may be possible to sub-class that
so that begin() would call a custom sub class begin(). but of course while this works it is going to add codes, memory and calls overheads. and it may have an unfortunate effect of making the codes 'hard to read'.
but c++ class based (or even template based) design are quite commonly used.
e.g. Adafruit abstracted various graphics drawing primitives into Adafruit GFX. and lcd chip specific libraries inherits Adafruit GFX and override call backs to implement the lcd specific ones. unfortunately it really is like
LCD_specific class inherits SPI LCD class (for SPI 'generic') inherits Adafruit GFX
and as the optimizations goes all the different 'duinos' are after all different and u'd find lots of if-defs even within the LCD_specific and SPI 'generic' parts

but this approach actually works and is successful if you observe the design pattern of Adafruit GFX
the complexity is unfortunate and cannot be avoided due to the hardware and api variations.
in effect there would be a separate class for each different LCD_specific subclass.
edit:
i may try to use this with my 'fork', i.e. the 'standard' functions calls the spi.h, spi.cpp 'official' definitions.
for users who 'really' want 'custom' spi, they would need to create a new instance like

CustomSPI myspi = CustomSPI()

and CustomSPI inherits SPIClass
then setup the 'library' to use CustomSPI instead of SPIClass
possibly by means of ifdefs - if probably can't be avoided

---

along this idea, eventual apis that can be formalized could be declared as virtual functions that needs to be overridden
e.g. dmaInit (callback from begin()), dmaSend and dmaReceive can be defined as calls to the 'standard' spi apis in the default api

[fpistm]

Aeduino SPI API compatibility have to be kept.
The class could be extended. For the DMA compatibility I have this in mind since a while and the better way would be to create an abstractionclass to provide generic DMA services. I think to extract DMA information from the STM32_open_pin_data

[ag88]

hi i think stas2z's ideas is sound and can be considered.
e.g. to introduce a method to return _spi;
other approaches which may be considered concurrently is to declare the private variables as protected, this is so that classes overriding SPIClass can access them

  protected:
    /* Contains various spiSettings for the same spi instance. Each spi spiSettings
    is associated to a CS pin. */
    SPISettings   spiSettings[NB_SPI_SETTINGS];

    // Use to know which configuration is selected.
    int16_t       _CSPinConfig;

    // spi instance
    spi_t         _spi;

then for methods such as

virtual void beginTransaction(uint8_t pin, SPISettings settings);
virtual void beginTransaction(SPISettings settings)
virtual void endTransaction(uint8_t pin);
virtual void endTransaction(void);
virtual byte transfer(uint8_t pin, uint8_t _data, SPITransferMode _mode = SPI_LAST);
virtual uint16_t transfer16(uint8_t pin, uint16_t _data, SPITransferMode _mode = SPI_LAST);
virtual void transfer(uint8_t pin, void *_buf, size_t _count, SPITransferMode _mode = SPI_LAST);
virtual void transfer(byte _pin, void *_bufout, void *_bufin, size_t _count, SPITransferMode _mode = SPI_LAST);
virtual byte transfer(uint8_t _data, SPITransferMode _mode = SPI_LAST)
virtual uint16_t transfer16(uint16_t _data, SPITransferMode _mode = SPI_LAST)
virtual void transfer(void *_buf, size_t _count, SPITransferMode _mode = SPI_LAST)
virtual void transfer(void *_bufout, void *_bufin, size_t _count, SPITransferMode _mode = SPI_LAST)

they can be declared virtual. the notion is so that SPIClass can be overridden with a custom class to provide features that's not otherwise bundled in the 'standard' SPIClass (e.g. dma etc).
virtual functions are necessary as otherwise calling the same methods using an inherited class cast to the base class would call the original base functions instead.

i think inherited classes probably has various overheads, including more sram use, additional over heads every call etc. but i'd think this may be worth trying so that 'experiments' can be started for additional features with spi.h, spi.cpp such as dma. eventually designs that works across most/all socs can eventually be merged back to the original spi.h, spi.cpp

[jefflessard]

I faced the same situation when I needed to access _spi to implement DMA transfer. I did not want to rewrite all the basic SPI code so I followed @ag88 idea and modified the SPIClass to allow derived class, in which I implemented the DMA.

@fpistm @ABOSTM, if it is of any interest, you can find the patch to apply this change here

If the client code of the library wants to set the SPI global variable to it's own implementation (derived class), then pass -DDERIVED_EXTERN_SPI when compiling the library.

The following header and code files demonstrate an example on how to derive SPIClass to implement such DMA feature.

[ABOSTM]

@ag88 , @jefflessard ,
Thanks for your proposal about virtual functions,
I tried it, and there is an overhead of about 1.2KB of FLASH only for SPI virtual functions (BarometricPressureSencor sketch on NUCLEO_l476RG).
Unfortunately this is too much for small MCU,
especially if we want to do the same for I2C and Serial.
and especially as only few advanced user will make use of it.
So proposal is to implement @stas2z original idea: add getters for internal HAL handle on SPI, I2C and Serial.