From 7abf1ba42be069bde4d3fc18851e011bab308531 Mon Sep 17 00:00:00 2001 From: "marco.accame" Date: Thu, 11 Mar 2021 10:45:49 +0100 Subject: [PATCH 1/9] hal_quadencoder: fixed bug which causes crash if function hal_quadencoder_init_indexes_flags() is called multiple times. - added protection inside hal_quadencoder_init_indexes_flags() so that the IRQ Handler in its inside is configured and started only if it was not already started or if it was stopped before. - added a new hal_quadencoder_deinit_indexes_flags() which stops the IRQ handler for a sefe use of multiple cycles of {init(), deinit()}. --- .../abslayer/hal2/api/hal_quadencoder.h | 9 ++++ .../hal2/src/extra/periphs/hal_quadencoder.c | 42 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/api/hal_quadencoder.h b/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/api/hal_quadencoder.h index 35e00ca87e..c27f85318d 100644 --- a/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/api/hal_quadencoder.h +++ b/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/api/hal_quadencoder.h @@ -90,9 +90,18 @@ extern uint32_t hal_quadencoder_get_counter(hal_quadencoder_t id); */ extern void hal_quadencoder_reset_counter(hal_quadencoder_t id); +/** @fn hal_quadencoder_deinit_indexes_flags(void) + @brief This function deinitialize the interrupt line of the index for the quadrature encoder + @param none + @retval none + */ +extern void hal_quadencoder_deinit_indexes_flags(void); /** @fn hal_quadencoder_init_indexes_flags(void) @brief This function initialize the interrupt line (which is not mandatory) of the index for the quadrature encoder + The funtion has a protecion vs multiple initializations. That is important because in its inside we configure + the start of an IRQ Handler which cannot be configured if it is running. To do a proper re-init call + hal_quadencoder_deinit_indexes_flags() first. @param none @retval none */ diff --git a/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/periphs/hal_quadencoder.c b/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/periphs/hal_quadencoder.c index 92ce10d7ea..5325036318 100644 --- a/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/periphs/hal_quadencoder.c +++ b/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/periphs/hal_quadencoder.c @@ -742,6 +742,20 @@ extern uint32_t hal_quadencoder_get_counter(hal_quadencoder_t id) return(temp); } +static uint8_t indexes_initted = 0; + +extern void hal_quadencoder_deinit_indexes_flags(void) +{ + + NVIC_InitTypeDef NVIC_InitStructure1; + NVIC_InitStructure1.NVIC_IRQChannel = EXTI15_10_IRQn; + NVIC_InitStructure1.NVIC_IRQChannelPreemptionPriority = 0x0F; + NVIC_InitStructure1.NVIC_IRQChannelSubPriority = 0x0F; + NVIC_InitStructure1.NVIC_IRQChannelCmd = DISABLE; + NVIC_Init(&NVIC_InitStructure1); + + indexes_initted = 0; +} extern void hal_quadencoder_init_indexes_flags(void) { @@ -750,6 +764,30 @@ extern void hal_quadencoder_init_indexes_flags(void) return; } + // marco.accame on 9 mar 2021: fix initialization of a hw peripheral with an active IRQ Handler. + // as a general rule, we should avoid touching the configuration of a peripheral which already + // has an active IRQ Handler because it may give undefined behaviour. + // that is what happened before this fix. the mc service calls this function + // multiple times and w/out protection that causes bad effects + // + // to fix we must either: + // 1. init just once and never init again. + // 2. make sure the IRQ is stopped, configure peripheral and then start IRQ + // (maybe also add a hal_quadencoder_deinit_indexes_flags() function which stops IRQ) + // + // + // solution + // i tested both solutions and both work: the calling application does not crash anymore. + // now hal_quadencoder_init_indexes_flags() has a protection vs multiple initializations + // and also there is a function which may deinit. + // it will be up to the user to use deinit() and then init() or call only init() as it is now + // done in EOtheMotionControl service + + if(1 == indexes_initted) + { + return; + } + // in future we should do it per quad-enc, thus .... // Indexes // @@ -787,6 +825,10 @@ extern void hal_quadencoder_init_indexes_flags(void) NVIC_InitStructure.NVIC_IRQChannelSubPriority = 0x0F; NVIC_InitStructure.NVIC_IRQChannelCmd = ENABLE; NVIC_Init(&NVIC_InitStructure); + + + indexes_initted = 1; + } From 2ef01a4a43efab203d29db488ee00689c2008ad2 Mon Sep 17 00:00:00 2001 From: "marco.accame" Date: Thu, 11 Mar 2021 10:49:57 +0100 Subject: [PATCH 2/9] hal_spiencoder: fixed a memory leak multiple cycles of {init(), deinit()} of the spiencoder wasted heap because the cleaning phase of deinit() was incomplete. now we either clean everything (if macro SPIENC_DEINIT_DEALLOCATE_HEAP is defined) or we try to keep the memory in order to avoid delete / new situations in runtime. (this latter is the default mode). --- .../abslayer/hal2/src/extra/devices/hal_mux.c | 3 + .../hal2/src/extra/devices/hal_spiencoder.c | 5 +- .../abslayer/hal2/src/extra/periphs/hal_spi.c | 112 +++++++++++++++--- .../libs/midware/hl-plus/api/hl_fifo.h | 4 + .../libs/midware/hl-plus/src/utils/hl_fifo.c | 35 ++++++ .../libs/midware/hl-plus/src/utils/hl_spi.c | 9 +- 6 files changed, 146 insertions(+), 22 deletions(-) diff --git a/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/devices/hal_mux.c b/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/devices/hal_mux.c index 89c3bd9928..5d7e4a2cb8 100644 --- a/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/devices/hal_mux.c +++ b/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/devices/hal_mux.c @@ -288,8 +288,11 @@ extern hal_result_t hal_mux_deinit(hal_mux_t id) } #endif s_hal_mux_initted_reset(id); +#if !defined(SPIENC_DEINIT_DEALLOCATE_HEAP) +#else hal_heap_delete((void**)&(s_hal_mux_theinternals.items[HAL_mux_id2index(id)])); //hal_heap_delete((void**)&intitem); +#endif return(hal_res_OK); } diff --git a/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/devices/hal_spiencoder.c b/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/devices/hal_spiencoder.c index 2d0d4920c0..02c97c7edf 100644 --- a/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/devices/hal_spiencoder.c +++ b/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/devices/hal_spiencoder.c @@ -820,10 +820,13 @@ extern hal_result_t hal_spiencoder_deinit(hal_spiencoder_t id) //Reset the flag associated to the encoder s_hal_spiencoder_initted_reset(id); - + +#if !defined(SPIENC_DEINIT_DEALLOCATE_HEAP) +#else //Dealloc all the associated memory hal_heap_delete((void**)&(s_hal_spiencoder_theinternals.items[HAL_encoder_id2index(id)])); //hal_heap_delete((void**)&intitem); +#endif return(hal_res_OK); } diff --git a/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/periphs/hal_spi.c b/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/periphs/hal_spi.c index 4fb3bb7feb..b9b7cd43a3 100644 --- a/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/periphs/hal_spi.c +++ b/emBODY/eBcode/arch-arm/libs/highlevel/abslayer/hal2/src/extra/periphs/hal_spi.c @@ -624,11 +624,46 @@ extern hal_result_t hal_spi_deinit(hal_spi_t id) return hal_res_NOK_generic; } - s_hal_spi_initted_reset(id); +#if !defined(SPIENC_DEINIT_DEALLOCATE_HEAP) + #warning SPIENC_DEINIT_DEALLOCATE_HEAP is not active so that we avoid runtime deallocation / allocation +#else + #warning SPIENC_DEINIT_DEALLOCATE_HEAP is active + + hal_spi_internal_item_t* intitem = s_hal_spi_theinternals.items[HAL_spi_id2index(id)]; - hal_heap_delete((void**)&(s_hal_spi_theinternals.items[HAL_spi_id2index(id)])); - //hal_heap_delete((void**)&intitem); + // we deallocate the pointers inside intitem + + if(NULL != intitem->rxFIFOisrframe) + { + hl_fifo_delete(intitem->rxFIFOisrframe); + intitem->rxFIFOisrframe = NULL; + } + if(NULL != intitem->txFIFOisrframe) + { + hl_fifo_delete(intitem->txFIFOisrframe); + intitem->txFIFOisrframe = NULL; + } + if(NULL != intitem->fiforx) + { + hl_fifo_delete(intitem->fiforx); + intitem->fiforx = NULL; + } + if(NULL != intitem->userdeftxframe) + { + hal_heap_delete((void**)&(intitem->userdeftxframe)); + intitem->userdeftxframe = NULL; + } + + // memset intitem to zero + memset(intitem, 0, sizeof(hal_spi_internal_item_t); + // and now delete it. + hal_heap_delete((void**)&(s_hal_spi_theinternals.items[HAL_spi_id2index(id)])); +#endif + + // we mark this spi as un-initted + s_hal_spi_initted_reset(id); + return (hal_res_OK); } @@ -919,14 +954,33 @@ static hal_result_t s_hal_spi_init(hal_spi_t id, const hal_spi_cfg_t *cfg) // -------------------------------------------------------------------------------------- // init the spi internals data structure + // marco.accame on 09mar2021 + // in here we implement the following policy: + // we allocate only if we find that pointers are either NULL or dont point to the required space + // in this way, multiple cycles of init() / deinit() may save some runtime allocations. + // to avoid as much as possible runtime allocations you should undefine macro SPIENC_DEINIT_DEALLOCATE_HEAP + // so that deinit() does not remove memory and at the next init() the pointers are still active. + + // we use it to understand if we need to reallocate some memory or not + size_t prevsizeoftxframe = 0; + // if it does not have ram yet, then attempt to allocate it. if(NULL == intitem) { intitem = s_hal_spi_theinternals.items[HAL_spi_id2index(id)] = hal_heap_new(sizeof(hal_spi_internal_item_t)); // minimal initialisation of the internal item - // nothing to init. - } - + // nothing to init. just reset everything even if hal_heap_new() already does it + memset(intitem, 0, sizeof(hal_spi_internal_item_t)); + prevsizeoftxframe = 0; + } + else + { + // we have an intitem from a previous call of s_hal_spi_init(). + // DONT memset(): intitem surely points to RAM alloated in a previous init() + // i get the size of tx frame in the previous initialization + prevsizeoftxframe = intitem->config.maxsizeofframe * intitem->sizeofword; + } + // - the config memcpy(&intitem->config, cfg, sizeof(hal_spi_cfg_t)); hal_spi_cfg_t *usedcfg = NULL; @@ -935,30 +989,50 @@ static hal_result_t s_hal_spi_init(hal_spi_t id, const hal_spi_cfg_t *cfg) // only frame-based intitem->sizeofframe = usedcfg->maxsizeofframe; intitem->sizeofword = (hal_spi_datasize_16bit == usedcfg->datasize) ? (2) : (1); + size_t sizeoftxframe = usedcfg->maxsizeofframe * intitem->sizeofword; - volatile uint8_t sss = intitem->sizeofword; - sss = sss; - + // - the isr rx frame (heap allocation) -#if defined(HAL_MANAGE_ISRFRAMES_WITH_FIFO) - intitem->rxFIFOisrframe = hl_fifo_new(usedcfg->maxsizeofframe, intitem->sizeofword, NULL); -#else +#if defined(HAL_MANAGE_ISRFRAMES_WITH_FIFO) + + // we use hl_fifo_new2() because ... if does not alloc memory if the fifo is already ok. + // it allocate ram only if th fifo is NULL, it realloc it if its required space has changed + intitem->rxFIFOisrframe = hl_fifo_new2(intitem->rxFIFOisrframe, usedcfg->maxsizeofframe, intitem->sizeofword, NULL); + +#else + #error -> you are using a not-tested mode: check vs this pointer being NULL ... intitem->rxBUFFERisrframe = (uint8_t*)hal_heap_new(usedcfg->maxsizeofframe * intitem->sizeofword); -#endif +#endif + // reset counter intitem->rxWORDScounter = 0; // - the isr tx frame (heap allocation) -#if defined(HAL_MANAGE_ISRFRAMES_WITH_FIFO) - intitem->txFIFOisrframe = hl_fifo_new(usedcfg->maxsizeofframe, intitem->sizeofword, NULL); - intitem->userdeftxframe = (uint8_t*)hal_heap_new(usedcfg->maxsizeofframe * intitem->sizeofword); -#else +#if defined(HAL_MANAGE_ISRFRAMES_WITH_FIFO) + // we use hl_fifo_new2() because ... if does not alloc memory if the fifo is already ok. + // it allocate ram only if th fifo is NULL, it realloc it if its required space has changed + intitem->txFIFOisrframe = hl_fifo_new2(intitem->txFIFOisrframe, usedcfg->maxsizeofframe, intitem->sizeofword, NULL); + + // the userdeftxframe must be allocated if NULL and reallocated if its required space has changed + if((NULL == intitem->userdeftxframe)) + { + intitem->userdeftxframe = (uint8_t*)hal_heap_new(sizeoftxframe); + } + else if(sizeoftxframe != prevsizeoftxframe) + { + // must reallocate. + hal_heap_delete((void**)&(intitem->userdeftxframe)); + intitem->userdeftxframe = (uint8_t*)hal_heap_new(sizeoftxframe); + } +#else + #error -> you are using a not-tested mode: check vs this pointer being NULL ... intitem->txBUFFERisrframe = (uint8_t*)hal_heap_new(usedcfg->maxsizeofframe * intitem->sizeofword); #endif // - the fifo of rx frames. but only if it is needed ... we dont need it if ... - intitem->fiforx = hl_fifo_new(usedcfg->capacityofrxfifoofframes, usedcfg->maxsizeofframe * intitem->sizeofword, NULL); - + #warning grosso come una casa: verifica la frase precedente + intitem->fiforx = hl_fifo_new2(intitem->fiforx, usedcfg->capacityofrxfifoofframes, usedcfg->maxsizeofframe * intitem->sizeofword, NULL); + // - the id intitem->id = id; diff --git a/emBODY/eBcode/arch-arm/libs/midware/hl-plus/api/hl_fifo.h b/emBODY/eBcode/arch-arm/libs/midware/hl-plus/api/hl_fifo.h index 708a978ed5..e623a2377b 100644 --- a/emBODY/eBcode/arch-arm/libs/midware/hl-plus/api/hl_fifo.h +++ b/emBODY/eBcode/arch-arm/libs/midware/hl-plus/api/hl_fifo.h @@ -98,6 +98,10 @@ struct hl_fifo_hid_t **/ extern hl_fifo_t* hl_fifo_new(uint8_t capacity, uint8_t sizeofitem, uint8_t *iteminitval); +// if fifo is NULL then it is just a hl_fifo_new(capacity, sizeofitem, iteminitval). else it reshapes the fifo. +// it reallocates RAM only if required: if (capacity*sizeofitem) and (fifo->capacity*fifo->sizeofitem) differ +extern hl_fifo_t* hl_fifo_new2(hl_fifo_t *fifo, uint8_t capacity, uint8_t sizeofitem, uint8_t *iteminitval); + /** @fn extern void hl_fifo_delete(hl_fifo_t *fifo) @brief It releases all memory related to the fifo. diff --git a/emBODY/eBcode/arch-arm/libs/midware/hl-plus/src/utils/hl_fifo.c b/emBODY/eBcode/arch-arm/libs/midware/hl-plus/src/utils/hl_fifo.c index a21c721bf1..bd86cbe754 100644 --- a/emBODY/eBcode/arch-arm/libs/midware/hl-plus/src/utils/hl_fifo.c +++ b/emBODY/eBcode/arch-arm/libs/midware/hl-plus/src/utils/hl_fifo.c @@ -133,6 +133,41 @@ extern hl_fifo_t* hl_fifo_new(uint8_t capacity, uint8_t sizeofitem, uint8_t *ite return(fifo); } +extern hl_fifo_t* hl_fifo_new2(hl_fifo_t *fifo, uint8_t capacity, uint8_t sizeofitem, uint8_t *iteminitval) +{ + if(NULL == fifo) + { + return hl_fifo_new(capacity, sizeofitem, iteminitval); + } + + // else we already have a fifo which we should ... either reinit (if capacity and sizeoitem are the same) or realloc. + if((0 == capacity) || (0 == sizeofitem)) + { + // as it is ... + return(fifo); + } + + if(((sizeofitem*capacity) == (fifo->sizeofitem*fifo->capacity)) && (NULL != fifo->data)) + { + // no realloc is required. i just ... reinit things + uint8_t *dataforfifo = fifo->data; + hl_fifo_init(fifo, capacity, sizeofitem, dataforfifo, iteminitval); + } + else if(NULL != fifo->data) + { + // need to realloc fifo->data + hl_sys_heap_delete(fifo->data); + fifo->data = NULL; + uint8_t *dataforfifo = hl_sys_heap_new(sizeofitem*capacity); + dataforfifo = hl_sys_heap_new(sizeofitem*capacity); + hl_fifo_init(fifo, capacity, sizeofitem, dataforfifo, iteminitval); + } + + return fifo; + +} + + extern void hl_fifo_delete(hl_fifo_t *fifo) { if(NULL == fifo) diff --git a/emBODY/eBcode/arch-arm/libs/midware/hl-plus/src/utils/hl_spi.c b/emBODY/eBcode/arch-arm/libs/midware/hl-plus/src/utils/hl_spi.c index dbbd1904de..2ff47fcaa5 100644 --- a/emBODY/eBcode/arch-arm/libs/midware/hl-plus/src/utils/hl_spi.c +++ b/emBODY/eBcode/arch-arm/libs/midware/hl-plus/src/utils/hl_spi.c @@ -223,7 +223,8 @@ extern hl_result_t hl_spi_init(hl_spi_t id, const hl_spi_cfg_t *cfg) if(NULL == intitem) { // the internal item - intitem = s_hl_spi_theinternals.items[HL_spi_id2index(id)] = hl_sys_heap_new(sizeof(hl_spi_internal_item_t)); + intitem = s_hl_spi_theinternals.items[HL_spi_id2index(id)] = hl_sys_heap_new(sizeof(hl_spi_internal_item_t)); + memset(intitem, 0, sizeof(hl_spi_internal_item_t)); } // set config @@ -400,9 +401,13 @@ extern hl_result_t hl_spi_deinit(hl_spi_t id) SPI_I2S_DeInit(SPIx); s_hl_spi_initted_reset(id); - //hal_heap_delete((void**)&(s_hl_spi_theinternals.items[HL_spi_id2index(id)])); + +#if !defined(SPIENC_DEINIT_DEALLOCATE_HEAP) +#else + // deallocation of internal pointers: none. hl_sys_heap_delete((void*)s_hl_spi_theinternals.items[HL_spi_id2index(id)]); memset(&s_hl_spi_theinternals.items[HL_spi_id2index(id)], 0, sizeof(hl_spi_internal_item_t*)); +#endif return hl_res_OK; } From 84159c0ed2b2154e9a622b01d425e0f482fee74d Mon Sep 17 00:00:00 2001 From: "marco.accame" Date: Thu, 11 Mar 2021 10:52:22 +0100 Subject: [PATCH 3/9] EOCurrentsWatchdog: removed a memory leak The function eo_currents_watchdog_Initialise() is called several times in runtime and did not have a protection for it and kept on allocating small chunks of memory. Solved that by using static RAM. --- .../v2/src/eoappservices/EOCurrentsWatchdog.c | 84 ++++++++++++++++++- .../eoappservices/EOCurrentsWatchdog_hid.h | 44 +++++++++- 2 files changed, 125 insertions(+), 3 deletions(-) diff --git a/emBODY/eBcode/arch-arm/board/ems004/appl/v2/src/eoappservices/EOCurrentsWatchdog.c b/emBODY/eBcode/arch-arm/board/ems004/appl/v2/src/eoappservices/EOCurrentsWatchdog.c index 58a6aeca91..04df20324b 100644 --- a/emBODY/eBcode/arch-arm/board/ems004/appl/v2/src/eoappservices/EOCurrentsWatchdog.c +++ b/emBODY/eBcode/arch-arm/board/ems004/appl/v2/src/eoappservices/EOCurrentsWatchdog.c @@ -104,6 +104,21 @@ EO_static_inline eObool_t s_eo_currents_watchdog_averageCalc_collectDataIsComple // - definition (and initialisation) of static variables // -------------------------------------------------------------------------------------------------------------------- + +#if defined(EOCURRENTSWATCHDOG_CORRECT_THE_INITIALISE_BUG) +static EOCurrentsWatchdog s_eo_currents_watchdog = +{ + EO_INIT(.themotors) {NULL, NULL, NULL, NULL}, + EO_INIT(.numberofmotors) 0, + //EO_INIT(.filter_reg) NULL, + EO_INIT(.nominalCurrent2) {0, 0, 0, 0}, + EO_INIT(.I2T_threshold) {0, 0, 0, 0}, + EO_INIT(.avgCurrent) {0, 0, 0, 0}, + EO_INIT(.accomulatorEp) {0, 0, 0, 0}, + EO_INIT(.initted) eobool_false, + EO_INIT(.motorinI2Tfault) {eobool_false, eobool_false, eobool_false, eobool_false} +}; +#else static EOCurrentsWatchdog s_eo_currents_watchdog = { EO_INIT(.themotors) NULL, @@ -116,11 +131,12 @@ static EOCurrentsWatchdog s_eo_currents_watchdog = EO_INIT(.initted) eobool_false, EO_INIT(.motorinI2Tfault) NULL }; +#endif static uint16_t suppliedVoltage_counter = 0; static eOmc_controller_t *nv_controller_ptr = NULL; #define SUMMPLIED_VOLTAGE_COUNTER_MAX 500 -//static const char s_eobj_ownname[] = "EOCurrentsWatchdog"; +static const char s_eobj_ownname[] = "EOCurrentsWatchdog"; static int16_t maxReadCurrent[4] ={15, 15, 15, 15};//15 mA is the cureent with no load see datasheet of motor (debug only) @@ -132,6 +148,57 @@ static int16_t maxReadCurrent[4] ={15, 15, 15, 15};//15 mA is the cureent with n extern EOCurrentsWatchdog* eo_currents_watchdog_Initialise(void) { + +#if defined(EOCURRENTSWATCHDOG_CORRECT_THE_INITIALISE_BUG) + + uint8_t m = 0; + + + // marco.accame on 09 mar 2021 + // the folloing part could go inside a eo_currents_watchdog_Activate() + // as in here there is just a preparation of static ram. + + // get the number of used motors + s_eo_currents_watchdog.numberofmotors = eo_entities_NumOfMotors(eo_entities_GetHandle()); + + if((0 == s_eo_currents_watchdog.numberofmotors) || ( s_eo_currents_watchdog.numberofmotors > maxmotors)) + { + s_eo_currents_watchdog.numberofmotors = 0; + return(&s_eo_currents_watchdog); + } + + + // retrieve pointers to motors + for(m=0; m