igor-krechetov / hsmcpp

C++ based Hierarchical / Finite State Machine library oriented for embedded and RTOS systems.
https://hsmcpp.readthedocs.io
MIT License
62 stars 7 forks source link

MCU compilation #1

Closed Germwalker closed 2 years ago

Germwalker commented 2 years ago

Hello,

I am trying to implement your HSM inside a STM32G4, ARM MCU. However i need to write a scheduler as i can't use the std::thread posix stuff on a Cortex-M4 CPU.

FreeRTOS can be used as OS to maintain asynchrony with the lib cpp_freertos wrote by Mickael Becker. This keep things very similar to the std::thread POSIX environnement. The major difference is the way threads are created and launched as cpp_freertos use a java like pattern with subclass and virtual run() method for threads.

I have (not so) sucessfully ported the HsmEventDispatcherBase.hpp/cpp files to HsmEventDispatcherBaseFREERTOS.hpp/cpp, also HsmEventDispatcherSTD.hpp/cpp files to HsmEventDispatcherFREERTOS.hpp/cpp with the cpp_freertos threading API.

I am now stuck with the hsm.hpp file here :

template <typename HsmStateEnum, typename HsmEventEnum>
void HierarchicalStateMachine<HsmStateEnum, HsmEventEnum>::PendingEventInfo::wait(const int timeoutMs)
{
    if (isSync())
    {
        std::unique_lock<std::mutex> lck(*cvLock);

        if (timeoutMs > 0)
        {
            syncProcessed->wait_for(lck, std::chrono::milliseconds(timeoutMs),
                                    [=](){return (HsmEventStatus_t::PENDING != *transitionStatus);});
        }
        else
        {
            syncProcessed->wait(lck, [=](){return (HsmEventStatus_t::PENDING != *transitionStatus);});
        }
    }

With cpp_freertos API that could become (i don't need the timeout) :

template <typename HsmStateEnum, typename HsmEventEnum>
void HierarchicalStateMachine<HsmStateEnum, HsmEventEnum>::PendingEventInfo::wait()
{
    if (isSync())
    {
        cpp_freertos::LockGuard lck(cvLock);
        Wait(syncProcessed, cvLock);
    }
}

You call syncProcessed->wait and that's a std::condition_variable method usable outside a thread (on a bare metal POSIX the main loop is also a thread indeed, not on a MCU). However, my cpp_freertos::Thread::Wait method need to be called inside the scope of a thread. So here i'm stuck, i don't know how i can fix it.

Maybe i can subclass HierarchicalStateMachine to run it inside a thread ?

Looking for help on this.

igor-krechetov commented 2 years ago

Hello @Germwalker ,

I had a look at this issue and I think I have a solution which might work for you. I had some concerns about compatibility when using concurrency objects from std library directly, but since I didn't have any need for support of non-posix systems, which don't have full support for C++11, I let it slide.

So to add FreeRTOS support it would be necessary to make an abstraction layer for threading objects and use it in HierarchicalStateMachine and HsmEventDispatcherBase classes. Then you will need to:

  1. implement thread synchronization objects using FreeRTOS API (I'm currently considering https://www.freertos.org/RTOS-task-notification-API.html or https://www.freertos.org/a00113.html APIs). Using cpp_freertos classes will not help here, but (judging by their code) there should be no conflict with low-level API.
  2. implement your own HsmEventDispatcherFreeRTOS. It should only require creation of a Task for dispatching events and handling timers (though this part is optional if you don't need this feature). You should have no need to port HsmEventDispatcherBase since it will also use abstraction layer and will be compatible.

Abstraction layer doesn't look too difficult, so, I think, I can implement it soon (I think it will be good to have regardless of FreeRTOS support).

I haven't used FreeRTOS before, so I wonder if you had any issues when using STL containers such as vector, list, function, map, etc.

Germwalker commented 2 years ago

Hello @igor-krechetov,

It would be indeed a pretty nice feature for the open source embedded community to have a reliable, small footprint (TinyFSM for example is bigger than 150Ko ...) and real time HSM. To make it real time compatible the events stimulus could be launched from ISR with FreeRTOS specials functions : ( vTaskNotifyGiveFromISR() for example ). HSMCPP could fit the needs and beyond.

Here is my try to port HSM to FreeRTOS with _cppfreertos (i have commented out the problematic lines in hsm.h) : hmscpp.zip

As you can see i subclassed the HsmEventDispatcherFREERTOS class to be able to run as a Thread with it's run() method :


#include "HsmEventDispatcherBaseFREERTOS.hpp"
#include "thread.hpp"
#include "condition_variable.hpp"

using namespace cpp_freertos;

namespace hsmcpp
{
    class HsmEventDispatcherFREERTOS : public HsmEventDispatcherBaseFREERTOS, public Thread
    {
    public:
        HsmEventDispatcherFREERTOS() : Thread(100, 1) { Start(); };
        virtual ~HsmEventDispatcherFREERTOS();

        virtual void emitEvent(const HandlerID_t handlerID) override;

        virtual bool start() override;
        void stop();

    protected:
        void doDispatching();

        // thread runtime
        virtual void Run();

    private:
        cpp_freertos::ConditionVariable mEmitEvent;
        cpp_freertos::MutexStandard mStartupSync;
        cpp_freertos::ConditionVariable mStartupEvent;

        bool mStopDispatcher = true; // unless we launch the thread that call start(), dispatcher is stalled

    };
} // namespace hsmcpp

My implementation is crashing as the dispatcher initialisation and runtime steps are not segregated (using the switch example) :

// HSM Scheduler engine instanciation
  std::shared_ptr<HsmEventDispatcherFREERTOS> dispatcher = std::make_shared<HsmEventDispatcherFREERTOS>();

  // HSM Application instanciation
  SwitchHsm hsm;
  hsm.initialize(dispatcher); // OS CRASH HERE
  hsm.transition(SwitchHsmEvents::SWITCH); // registering events

  // start the thread c++ freertos implementation scheduler
  Thread::StartScheduler();

As "Thread::StartScheduler();" (or "osKernelStart();" in bare metal FreeRTOS) are called after initializing and using threaded objects inside HsmEventDispatcherFREERTOS.cpp, FreeRTOS crash because the scheduler needs to be started first to use threaded objects.

I agree that :

As i experimented it, there is any problem to use STL containers in embedded FreeRTOS programs. Only the threading stuff is missing because it's only a stub.

igor-krechetov commented 2 years ago

Hello @Germwalker , I checked your code and the crash seems to happen due to incorrect implementation the dispatcher. start() method is supposed to look something like this:

bool HsmEventDispatcherFREERTOS::start()
{
    __HSM_TRACE_CALL_DEBUG__();
    bool result = true;
        if (thread is not running)
    {
        Start();
    }
    return result;// need to check if thread was started or not
}

Currently you automatically start your thread in constructor (which is okay, though might be earlier than actually needed). Then thread would call Run() which in turn will try to start the thread again:

void HsmEventDispatcherFREERTOS::Run()
{
    __HSM_TRACE_CALL_DEBUG__();
    this->Start();
}

Instead Run() should have had logic from doDispatching().

And in your code this happens (on the same thread):

call hsm.initialize() -> dispatcher.start() -> dispatcher.doDispatching() -> Wait()

And I assume that's where the crash happens because Thread::StartScheduler() was not called yet.

I already started implementation (that I mentioned in my previous message), but since I never used FreeRTOS would be good if you could help me clarify a couple of things:

  1. How different is FreeRTOS behavior when build and run on Linux system instead of MCU? Any pitfalls I should keep in mind when testing?
  2. Where the instance of hsmcpp should be created and initialized: in main function or inside of a new Task?
  3. Is there a reliable way to know that current function is being executed from inside of ISR? If found xPortIsInsideInterrupt() API, but it's availability seems to depend on the MCU port.
  4. My current understanding of FreeRTOS task notifications is that it's necessary to specify which exact task you are notifying. So there is no broadcast behavior which is provided by semaphores/mutexes. Correct?
  5. I read that when using C++ with FreeRTOS it's important to redefine new/delete operators. Are there any out-of-the-box solutions for that or every developer has to do it by themselves?
  6. Do you plan to use state machine only in async mode or you also will need synchronous transitions?
Germwalker commented 2 years ago

Hello @igor-krechetov ,

Thanks for the code analysis ! I'm basicaly a hardware engineer, still got a lot to learn. There is also a confusion inside my port as start() belong to HsmEventDispatcherFREERTOS and Start() belong to cpp_freertos::Thread and is used to start the thread if the scheduler is active, that's why i called it from the constructor.

Here my answers to your questions :

  1. I was actualy looking for a way to build a FreeRTOS software on linux as not everybody own a ARM MCU dev kit. There is a POSIX emulator for FreeRTOS that we could use for the dev & test. I can also use my STM32G4 dev kit to validate the design on the real hardware.

[EDIT] : i just launched the Posix_GCC example, it run great :

git clone https://github.com/FreeRTOS/FreeRTOS
cd FreeRTOS
git submodule update --init --recursive
cd FreeRTOS/Demo/Posix_GCC
make -j16
./build/posix_demo

You're up and running.

Some basic mistakes on FreeRTOS :

  1. The hsmcpp object contains threaded stuff :

    syncProcessed->wait(lck, [=](){return (HsmEventStatus_t::PENDING != *transitionStatus);});

    so as for me it should belong to a task context. So we're talking about 2 tasks, one for the hsmcpp object and one for the dispatcher.

  2. In a real time context an interupt function should remain as fast as possible because the CPU is locked during the ISR and cannot be preempted. The best option should be binding a hardware event (switch GPIO triggered for example) and sending it to the dispatcher inside the ISR context, then leave. This way the ISR is rapidly exited and the scheduler is fully in control of the real time event as tasks can be prioritized from hard real time (high value) to soft real time (low value, idle task is 0).

[EDIT] : a solution could be launching a specific transition_fromISR method that notify the scheduler using _fromISR API and not checking the context.

  1. You're correct, according to the FreeRTOS documentation :

    RTOS task notifications can only be used when there is only one task that can be the recipient of the event. This condition is however met in the majority of real world use cases, such as an interrupt unblocking a task that will process the data received by the interrupt.

    Only in the case where an RTOS task notification is used in place of a queue: While a receiving task can wait for a notification in the Blocked state (so not consuming any CPU time), a sending task cannot wait in the Blocked state for a send to complete if the send cannot complete immediately.

  2. You will indeed need to wrap pvPortMalloc(size) and vPortFree(ptr) to use new / delete operators in FreeRTOS. Keep in mind that we cannot use dynamic allocation inside MCU so you must not use delete (it could be implemented but it will not be used i a real context) and call all the new memory allocations during the init phase.

  3. What you call synchronous transitions are timers and stimulus with timeouts, right ? Timers are mandatory, timelout are less, it's up to you.

igor-krechetov commented 2 years ago

@Germwalker , I added experimental support for FreeRTOS. Basic functionality works with POSIX emulator. Please try to build and run this example with your MCU: https://github.com/igor-krechetov/hsmcpp/tree/experimental_freertos/examples/08_freertos

To try with POSIX emulator you will need to specify path to FreeRTOS files in CMakeLists.txt and run build.sh script from 08_freertos folder:

SET(FREERTOS_ROOT "~/projects/FreeRTOS")

Additionally to configuring your toolchain for MCU build, you might need to modify these CMakeLists commands:

# path for portable files should be changed
SET(FREERTOS_INCLUDE ${FREERTOS_KERNEL_DIR}/include
                     ${FREERTOS_KERNEL_DIR}/portable/ThirdParty/GCC/Posix
                     ${FREERTOS_KERNEL_DIR}/portable/ThirdParty/GCC/Posix/utils
                     ${FREERTOS_PLUS_DIR}/Source/FreeRTOS-Plus-Trace/Include)

# need to fix value of FREERTOS_PORT and FREERTOS_HEAP (if needed)
ExternalProject_Add(FREERTOS
                    SOURCE_DIR "${FREERTOS_KERNEL_DIR}"
                    BINARY_DIR "${FREERTOS_LIB_DIR}"
                    INSTALL_DIR ""
                    CMAKE_ARGS "-DFREERTOS_CONFIG_FILE_DIRECTORY=${CMAKE_CURRENT_SOURCE_DIR}"
                               "-DFREERTOS_HEAP=3"
                               "-DFREERTOS_PORT=GCC_POSIX"
                    INSTALL_COMMAND ""
                    )

I would appreciate if you could additionally:

Germwalker commented 2 years ago

Heya @igor-krechetov,

I just built your experimental branch on a POSIX system.

I had to add this two libs to be able to build :

It's working well on POSIX, i can see the tasks switching :

[iridium@fedora build]$ ./08_freertos 
BEGIN
[PID:17872, TID:17872] HsmEventDispatcherFreeRTOS::start: [DEBUG]  was called
[PID:17872, TID:17872] HsmEventDispatcherFreeRTOS::start: [DEBUG] starting task...
[PID:17872, TID:17872] HsmEventDispatcherBase::registerEventHandler: [DEBUG]  was called
vTaskStartScheduler
[PID:17872, TID:17875] HsmEventDispatcherFreeRTOS::doDispatching: [DEBUG]  was called
[PID:17872, TID:17875] HsmEventDispatcherBase::dispatchPendingEvents: [DEBUG]  was called
[PID:17872, TID:17875] HsmEventDispatcherFreeRTOS::doDispatching: [DEBUG] wait for emit...
taskTransitions - BEGIN
SwitchHsmEvents::SWITCH --> HSM
[PID:17872, TID:17876] HsmEventDispatcherFreeRTOS::emitEvent: [DEBUG]  was called
[PID:17872, TID:17876] HsmEventDispatcherBase::emitEvent: [DEBUG]  was called
[PID:17872, TID:17875] HsmEventDispatcherFreeRTOS::doDispatching: [DEBUG] woke up. pending events=1
[PID:17872, TID:17875] HsmEventDispatcherBase::dispatchPendingEvents: [DEBUG]  was called
Off
[PID:17872, TID:17875] HsmEventDispatcherFreeRTOS::doDispatching: [DEBUG] wait for emit...

I tried to build the state machine for my MCU : here is the full project updated.

I only had to add #include "memory" to hsm.hpp to build the project as it was needed for std::shared_ptr.

I used this compiler flags :

The project build sucessfully and i can flash the MCU correctly.

I used the taskTransition() as you specified it :

void taskTransitions(void *pvParameters)
{
  SwitchHsm *hsm = static_cast<SwitchHsm *>(pvParameters);

  while (true)
  {
    vTaskDelay(1000 / portTICK_PERIOD_MS);

    if (true == hsm->isInitialized())
    {
      hsm->transition(SwitchHsmEvents::SWITCH);
    }
  }
  vTaskDelete(nullptr);
}

I also tried to replicate as much as possible your FreeRTOSConfig.h, you can find the one i built here. However, i'm falling into a hardfault event, here is the call stack i'm stuck in :

HardFault_Handler@0x0801bcb0 (g4-hsmcpp-freertos\Core\Src\stm32g4xx_it.c:94)
<signal handler called>@0xfffffffd (Source inconnue:0)
vListInsertEnd@0x0801d8a6 (g4-hsmcpp-freertos\Middlewares\Third_Party\FreeRTOS\Source\list.c:76)
xTaskPriorityDisinherit@0x08020134 (g4-hsmcpp-freertos\Middlewares\Third_Party\FreeRTOS\Source\tasks.c:4149)
prvCopyDataToQueue@0x0801eece (g4-hsmcpp-freertos\Middlewares\Third_Party\FreeRTOS\Source\queue.c:2090)
xQueueGenericSend@0x0801e5e2 (g4-hsmcpp-freertos\Middlewares\Third_Party\FreeRTOS\Source\queue.c:833)
hsmcpp::Mutex::unlock@0x08005954 (g4-hsmcpp-freertos\Core\Lib\hsmcpp\include\os\freertos\concurrency.hpp:55)
hsmcpp::LockGuard::~LockGuard@0x08005992 (g4-hsmcpp-freertos\Core\Lib\hsmcpp\os\common\concurrency.cpp:15)
hsmcpp::HsmEventDispatcherBase::dispatchPendingEvents@0x080021da (g4-hsmcpp-freertos\Core\Lib\hsmcpp\HsmEventDispatcherBase.cpp:197)
hsmcpp::HsmEventDispatcherFreeRTOS::doDispatching@0x08005802 (g4-hsmcpp-freertos\Core\Lib\hsmcpp\HsmEventDispatcherFreeRTOS.cpp:86)
pxPortInitialiseStack@0x0801da10 (g4-hsmcpp-freertos\Middlewares\Third_Party\FreeRTOS\Source\portable\GCC\ARM_CM4F\port.c:214)

As you can see there is a problem with a mutex inside hsm.hpp :

void unlock()
    {
        if (nullptr != mHandle)
        {
            xSemaphoreGive(mHandle);  // <-- HERE
        }
    }

The n-1 final crashing line is here inside the task.c file :

prvAddTaskToReadyList( pxTCB );

Here is the content of the task pxTCB (it's name is HsmEventDispatcher) : 10

The mutex has been released indeed.

This is the n crashing line :

void vListInsertEnd( List_t * const pxList, ListItem_t * const pxNewListItem )
{
ListItem_t * const pxIndex = pxList->pxIndex; // <-- HERE

Here is the content of pxIndex & pxIndex : 11

[EDIT] : i just tried with a STM32F767, same hardfault at the same place. I'm looking at the hardfault registers to have more informations about the root problem.

My heap size is 100ko, shoud be sufficient. I tested with 200ko on the F7, same result.

I'm working on the transition call from ISR & hooks.c / FreeRTOSConfig.h optimisation. Once we can make it work i'll try to launch with the bare minimal FreeRTOSConfig.h.

igor-krechetov commented 2 years ago

@Germwalker , I can see what caused the crash, but it's still not clear how it happened. If you look at the pxTCB values you'll see that uxPriority and uxBasePriority have value 536873800. This is abnormal since priority should never be more than configMAX_PRIORITIES (which is 56 in current case). As a result this line in tasks.c (222) crashes:

vListInsertEnd( &( pxReadyTasksLists[ ( pxTCB )->uxPriority ] ), &( ( pxTCB )->xStateListItem ) );

pxReadyTasksLists is a static array of size configMAX_PRIORITIES and out of boundary memory is being passed and accessed.

Hard to say about the root-cause since I don't have a way to debug it. I did found something related in FAQ: https://www.freertos.org/RTOS-Cortex-M3-M4.html

Most systems default to the wanted configuration, with the noticeable exception of the STM32 driver library. If you are using an STM32 with the STM32 driver library then ensure all the priority bits are assigned to be preempt priority bits by calling NVIC_PriorityGroupConfig( NVIC_PriorityGroup_4 ); before the RTOS is started.

Since configUSE_PREEMPTION is defined, I guess, it might be worth checking.

Other things you can try are:

And regarding ISR, make sure that HSMBUILD_FREERTOS_DEFAULT_ISR_DETECT build option is set to OFF. This flag is mostly for PC simulation.

Germwalker commented 2 years ago

Hello @igor-krechetov,

The hardfault was indeed a combinaison of factors, including a minimal stack size to weak and a bad initialisation of the hsm & dispatcher objects inside the main(). A minimal stack size of 512 words work flawlessly. I've managed to get the HSM output debug working, it is by now integrated with my serial console (TX DMA is used). It should be compatible with every STM32 lines. The heap size target 15ko but i have some more stuff for the serial console, anyway its's very conservative on RAM.

With -O0 optimisation the firmware is ~200ko, with -02 it's around ~90ko. The HSM alone weight about 60ko, it's heavy but nowadays 250ko is quite cheap. Maybe we can lighten it for deeply embedded applications but it's not the end of the world.

I made this changes to the logging.hpp file. For now only the led blinking example is functionnal but i plan to make a real machine with it. A coffee machine example could be nice, i'll see if i have some time for it.

I also minified the FreeRTOSConfig.h, the configuration is very similar to the default initialisation of CubeMX appart for the minimal stack size and heap size.

Here are my build flags :

  - DHSMBUILD_DISPATCHER_QT=OFF
  - DHSMBUILD_DISPATCHER_GLIB=OFF
  - DHSMBUILD_DISPATCHER_GLIBMM=OFF
  - HSM_ENABLE_SAFE_STRUCTURE
  - PLATFORM_FREERTOS
  - HSMBUILD_DISPATCHER_FREERTOS
  - HSM_LOGGING_MODE_STRICT_VERBOSE 

I modified the task transition this way :

void hsmTransitions(void *args)
{
  hsm.initialize(dispatcher);

  while (true)
  {
    if (true == hsm.isInitialized())
    {
      hsm.transition(SwitchHsmEvents::SWITCH);
      HAL_GPIO_TogglePin(LED_BLUE_GPIO_Port, LED_BLUE_Pin);
      osDelay(50);
    }
  }
  osThreadTerminate(NULL);
}

I'm directly using the hsm & dispatchers objects rather than creating a pointer to the structures, as a consequence the task is not given the HSM object, it's used from the global context :

xTaskCreate(hsmTransitions, "taskTransitions", 256, (void *)1, 3, &hsmTransitionsHandler);

So for now everything outside ISR is working fine !

Here a console debug sample log :

HsmEventDispatcherFreeRTOS::doDispatching: [DEBUG] woke up. pending events=1
HsmEventDispatcherBase::dispatchPendingEvents: [DEBUG]  was called
HSM::dispatchEvents: [DEBUG] mPendingEvents.size=1
HSM::doTransition: [DEBUG] event=<SWITCH>, transitionType=0
HSM::handleSingleTransition: [DEBUG] activeState=<Off>, event=<SWITCH>, transitionType=0
HSM::findTransitionTarget: [DEBUG] fromState=<Off>, event=<SWITCH>
HSM::findTransitionTarget: [DEBUG] check transition to <On>...
HSM::findTransitionTarget: [DEBUG]  => true
HSM::onStateExiting: [DEBUG] state=<Off>
HSM::executeStateAction: [DEBUG] state=<Off>, actionTrigger=1
HSM::onStateEntering: [DEBUG] state=<On>
HSM::executeStateAction: [DEBUG] state=<On>, actionTrigger=0
HSM::replaceActiveState: [DEBUG] oldState=<Off>, newState=<On>
HSM::addActiveState: [DEBUG] newState=<On>
HSM::addActiveState: [DEBUG] mActiveStates.size=1
HSM::onStateChanged: [DEBUG] state=<On>
HSM::handleSingleTransition: [DEBUG]  => 1
HSM::doTransition: [DEBUG]  => 1
HSM::dispatchEvents: [DEBUG] unlock with status 1
HSM::unlock: [DEBUG] try to unlock with status=1
HSM::unlock: [DEBUG] ASYNC object
HsmEventDispatcherFreeRTOS::doDispatching: [DEBUG] wait for emit...

I tried to call a transition from an ISR callback by using the HAL tick callback function (TIM2 is dedicated to the HSM and oscillate @1kHz) :

void HAL_TIM_PeriodElapsedCallback(TIM_HandleTypeDef *htim)
{
  /* USER CODE BEGIN Callback 0 */

  /* USER CODE END Callback 0 */
  if (htim->Instance == TIM1)
  {
    HAL_IncTick();
  }
  /* USER CODE BEGIN Callback 1 */
  else if (htim->Instance == TIM2 && true == hsm.isInitialized())
  {
    hsm.transition(SwitchHsmEvents::SWITCH);
  }
  /* USER CODE END Callback 1 */
}

I'm falling into this assert in the port.c FreeRTOS file :

void vPortEnterCritical( void )
{
    portDISABLE_INTERRUPTS();
    uxCriticalNesting++;

    /* This is not the interrupt safe version of the enter critical function so
    assert() if it is being called from an interrupt context.  Only API
    functions that end in "FromISR" can be used in an interrupt.  Only assert if
    the critical nesting count is 1 to protect against recursive calls if the
    assert function also uses a critical section. */
    if( uxCriticalNesting == 1 )
    {
        configASSERT( ( portNVIC_INT_CTRL_REG & portVECTACTIVE_MASK ) == 0 ); <-- HERE
    }
}

Here is the call stack :

vPortEnterCritical@0x080276de (g4-hsmcpp-freertos\Middlewares\Third_Party\FreeRTOS\Source\portable\GCC\ARM_CM4F\port.c:415)
xQueueSemaphoreTake@0x08028634 (g4-hsmcpp-freertos\Middlewares\Third_Party\FreeRTOS\Source\queue.c:1448)
hsmcpp::Mutex::lock@0x08006066 (g4-hsmcpp-freertos\Core\Lib\hsmcpp\include\os\freertos\concurrency.hpp:46)
hsmcpp::LockGuard::LockGuard@0x080060c2 (g4-hsmcpp-freertos\Core\Lib\hsmcpp\os\common\concurrency.cpp:10)
hsmcpp::HierarchicalStateMachine<SwitchHsmStates, SwitchHsmEvents>::transitionExWithArgsArray@0x08011e06 (g4-hsmcpp-freertos\Core\Lib\hsmcpp\include\hsm.hpp:1004)
hsmcpp::HierarchicalStateMachine<SwitchHsmStates, SwitchHsmEvents>::transitionEx<>(SwitchHsmEvents, bool, bool, int)@0x080111e2 (g4-hsmcpp-freertos\Core\Lib\hsmcpp\include\hsm.hpp:961)
hsmcpp::HierarchicalStateMachine<SwitchHsmStates, SwitchHsmEvents>::transition<>(SwitchHsmEvents)@0x080108bc (g4-hsmcpp-freertos\Core\Lib\hsmcpp\include\hsm.hpp:970)
HAL_TIM_PeriodElapsedCallback@0x08010454 (g4-hsmcpp-freertos\Core\Src\main.cpp:273)
HAL_TIM_IRQHandler@0x08022f6c (g4-hsmcpp-freertos\Drivers\STM32G4xx_HAL_Driver\Src\stm32g4xx_hal_tim.c:3881)
TIM2_IRQHandler@0x0801fc06 (g4-hsmcpp-freertos\Core\Src\stm32g4xx_it.c:288)
<signal handler called>@0xfffffffd (Source inconnue:0)
HAL_TIM_Base_Start_IT@0x08022b36 (g4-hsmcpp-freertos\Drivers\STM32G4xx_HAL_Driver\Src\stm32g4xx_hal_tim.c:487)
hsmTransitions@0x08010378 (g4-hsmcpp-freertos\Core\Src\main.cpp:220)
pxPortInitialiseStack@0x080274a0 (g4-hsmcpp-freertos\Middlewares\Third_Party\FreeRTOS\Source\portable\GCC\ARM_CM4F\port.c:214)

As you can see we are entering a critical section inside an ISR with the taskENTER_CRITICAL(); function just after locking the mutex :

locked = (pdTRUE == xSemaphoreTake(mHandle, portMAX_DELAY));

It's just a #define for :

#define xSemaphoreTake( xSemaphore, xBlockTime )        xQueueSemaphoreTake( ( xSemaphore ), ( xBlockTime ) )

And xQueueSemaphoreTake( ( xSemaphore ), ( xBlockTime ) ) uses only a taskENTER_CRITICAL(); section.

This page of the FreeRTOS documentation say something about this behavior :

FreeRTOS API functions must not be called from within a critical section.
taskENTER_CRITICAL() and taskEXIT_CRITICAL() **must not be called from an interrupt** service routine (ISR) - see [**taskENTER_CRITICAL_FROM_ISR()**](https://www.freertos.org/taskENTER_CRITICAL_FROM_ISR_taskEXIT_CRITICAL_FROM_ISR.html) and **taskEXIT_CRITICAL_FROM_ISR()** for interrupt safe equivalents.

There is a xSemaphoreTake( xSemaphore, xBlockTime ) equivalent for ISR :

xSemaphoreTakeFromISR
      (
        SemaphoreHandle_t xSemaphore,
        signed BaseType_t *pxHigherPriorityTaskWoken
      )

Many thanks for the FreeRTOS port, it's well writen and seems quite robust. I'll post more about the minifing of the hooks.c file.

igor-krechetov commented 2 years ago

@Germwalker , thank you for your test results and modifications. I integrated them in the latest commit.

Since FreeRTOS support seems to move forward, I fixed unittests build script to support FreeRTOS (Posix simulation). Found a couple issues related with new dispatcher synchronization which were fixed and overall cleaned up the new code. This also includes ISR fix for Mutex which you mentioned before. I was aware about xxxFromISR versions of the API, but got confused by xSemaphoreGive which states that FromISR version is not needed for Mutexes (which is not true for xSemaphoreTake).

Please try testing again transitions from ISR callbacks.

Regarding memory footprint, please try with these additional settings (for CMake build): -DHSMBUILD_DEBUGGING=OFF -DHSMBUILD_STRUCTURE_VALIDATION=OFF -DHSMBUILD_VERBOSE=OFF

If you build without CMake, then you will only need to have these: -DPLATFORM_FREERTOS -DHSM_BUILD_HSMBUILD_DISPATCHER_FREERTOS -DHSM_LOGGING_MODE_OFF

Verbosity of HMS was intended only for library development. And first two options were intended to help with development, but should be turned off in release version of the application.

I still have not added Timers support to FreeRTOS dispatcher. Since API doesn't look too hard I think it will not take too much time.

Germwalker commented 2 years ago

Hello @igor-krechetov ,

I integrated your last commit, ISR are now 80% working.

I made a machine that toggles two leds : 01

One is toggled by a task with a delay, the other one toggle on an interupt launched by TIM2. Both events occurs at 1Hz.

I toggle the first led here :

void hsmTransitions(void *args)
{
  hsm.initialize(dispatcher);

  while (true)
  {
    if (true == hsm.isInitialized())
    {
      hsm.transition(BlinkMachineEvents::SWITCH_ORANGE);
      osDelay(pulse_delay);
    }
  }
  osThreadTerminate(NULL);
}

and the second one here insire ISR callback :

void HAL_TIM_PeriodElapsedCallback(TIM_HandleTypeDef *htim)
{
   if (htim->Instance == TIM2 && true == hsm.isInitialized())
  {
    hsm.transition(BlinkMachineEvents::SWITCH_RED);
  }
}

I just had to transform a bit the mutex::unlock function (had an assert before complaining about critical section inside ISR) :

from

void Mutex::unlock()
{
    if (nullptr != mHandle)
    {
        xSemaphoreGive(mHandle);
    }
}

to

void Mutex::unlock()
{
    if (nullptr != mHandle)
    {
        if (pdTRUE == xPortIsInsideInterrupt())
        {
            xSemaphoreGiveFromISR(mHandle, nullptr);
        }
        else
        {
            xSemaphoreGive(mHandle);
        }
    }
}

Here is the assert is was falling in :

/* This is not the interrupt safe version of the enter critical function so
    assert() if it is being called from an interrupt context.  Only API
    functions that end in "FromISR" can be used in an interrupt.  Only assert if
    the critical nesting count is 1 to protect against recursive calls if the
    assert function also uses a critical section. */
    if( uxCriticalNesting == 1 )
    {
        configASSERT( ( portNVIC_INT_CTRL_REG & portVECTACTIVE_MASK ) == 0 );
    }

Then it worked quite well at 1Hz but freezed sometimes. To reproduce the stall i called the ISR at 20Hz (50ms), but then after a few seconds this part behaves as an eternal loop :

do
        {
            if (pdTRUE == isInsideISR)
            {
                locked = (pdTRUE == xSemaphoreTakeFromISR(mHandle, nullptr)); <- eternal loop here
            }
            else
            {
                locked = (pdTRUE == xSemaphoreTake(mHandle, portMAX_DELAY));
            }
        } while(false == locked);

The system seems to be stalled as i only have instructions about ISR running and the cpu is stuck inside the instruction. If i set a breakpoint on the line, it trigger it repeatedly but the system is stall.

Here is the call stack :

xQueueReceiveFromISR@0x0802768c (g4-hsmcpp-freertos\Middlewares\Third_Party\FreeRTOS\Source\queue.c:1792)
hsmcpp::Mutex::lock@0x0800e732 (g4-hsmcpp-freertos\Core\Lib\hsmcpp\os\freertos\Mutex.cpp:37)
hsmcpp::LockGuard::LockGuard@0x0800e382 (g4-hsmcpp-freertos\Core\Lib\hsmcpp\os\common\LockGuard.cpp:11)
hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::transitionExWithArgsArray@0x08012bb2 (g4-hsmcpp-freertos\Core\Lib\hsmcpp\include\hsm.hpp:1007)
hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::transitionEx<>(BlinkMachineEvents, bool, bool, int)@0x0801207c (g4-hsmcpp-freertos\Core\Lib\hsmcpp\include\hsm.hpp:964)
hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::transition<>(BlinkMachineEvents)@0x08011870 (g4-hsmcpp-freertos\Core\Lib\hsmcpp\include\hsm.hpp:973)
HAL_TIM_PeriodElapsedCallback@0x08011478 (g4-hsmcpp-freertos\Core\Src\main.cpp:254)
HAL_TIM_IRQHandler@0x08022d88 (g4-hsmcpp-freertos\Drivers\STM32G4xx_HAL_Driver\Src\stm32g4xx_hal_tim.c:3881)
TIM2_IRQHandler@0x0801fa8e (g4-hsmcpp-freertos\Core\Src\stm32g4xx_it.c:288)
<signal handler called>@0xfffffffd (Source inconnue:0)
__gnu_cxx::__normal_iterator<hsmcpp::Variant*, std::vector<hsmcpp::Variant, std::allocator<hsmcpp::Variant> > >::base@0x0800fd5c (c:\vsarm\armcc\arm-none-eabi\include\c++\10.3.1\bits\stl_iterator.h:1056)
std::__niter_base<hsmcpp::Variant*, std::vector<hsmcpp::Variant, std::allocator<hsmcpp::Variant> > >@0x080167be (c:\vsarm\armcc\arm-none-eabi\include\c++\10.3.1\bits\stl_iterator.h:1227)
std::__copy_move_a<false, __gnu_cxx::__normal_iterator<hsmcpp::Variant const*, std::vector<hsmcpp::Variant, std::allocator<hsmcpp::Variant> > >, __gnu_cxx::__normal_iterator<hsmcpp::Variant*, std::vector<hsmcpp::Variant, std::allocator<hsmcpp::Variant> > > >@0x08014c02 (c:\vsarm\armcc\arm-none-eabi\include\c++\10.3.1\bits\stl_algobase.h:513)
std::copy<__gnu_cxx::__normal_iterator<hsmcpp::Variant const*, std::vector<hsmcpp::Variant, std::allocator<hsmcpp::Variant> > >, __gnu_cxx::__normal_iterator<hsmcpp::Variant*, std::vector<hsmcpp::Variant, std::allocator<hsmcpp::Variant> > > >@0x08012e04 (c:\vsarm\armcc\arm-none-eabi\include\c++\10.3.1\bits\stl_algobase.h:569)
std::vector<hsmcpp::Variant, std::allocator<hsmcpp::Variant> >::operator=@0x08012358 (c:\vsarm\armcc\arm-none-eabi\include\c++\10.3.1\bits\vector.tcc:238)
hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::PendingEventInfo::operator=@0x08011b28 (g4-hsmcpp-freertos\Core\Lib\hsmcpp\include\hsm.hpp:193)
hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::dispatchEvents@0x08011bd8 (g4-hsmcpp-freertos\Core\Lib\hsmcpp\include\hsm.hpp:1132)
std::__invoke_impl<void, void (hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::*&)(), hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>*&>@0x0801b932 (c:\vsarm\armcc\arm-none-eabi\include\c++\10.3.1\bits\invoke.h:73)
std::__invoke<void (hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::*&)(), hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>*&>@0x0801aa4e (c:\vsarm\armcc\arm-none-eabi\include\c++\10.3.1\bits\invoke.h:95)
std::_Bind<void (hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::*(hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>*))()>::__call<void, , 0u>(std::tuple<>&&, std::_Index_tuple<0u>)@0x080192ea (c:\vsarm\armcc\arm-none-eabi\include\c++\10.3.1\functional:416)

I tried to modify the Mutex::lock() function this way :

if (pdTRUE == isInsideISR)
  {
      locked = (pdTRUE == xSemaphoreTakeFromISR(mHandle, nullptr));
  }
  else
  {
      locked = (pdTRUE == xSemaphoreTake(mHandle, portMAX_DELAY));
  }  

but i have then have this assertion :

/* Normally a mutex would not be given from an interrupt, especially if
    there is a mutex holder, as priority inheritance makes no sense for an
    interrupts, only tasks. */
    configASSERT( !( ( pxQueue->uxQueueType == queueQUEUE_IS_MUTEX ) && ( pxQueue->u.xSemaphore.xMutexHolder != NULL ) ) );

So i guess this modification is not the solution. At 20Hz the crash or bootloop occurs after ~30seconds. Same behavior on Cortex-M4 & Cortex-M7.

Maybe it's when ISR and task transition occurs at the same time ?

The FreeRTOS documentation say something about using mutex inside ISR :

Mutexes should not be used from an interrupt because: They include a priority inheritance mechanism which only makes sense if the mutex is given and taken from a task, not an interrupt. An interrupt cannot block to wait for a resource that is guarded by a mutex to become available.

I don't know if it's related to the problem as the program execute nicely for a couple of seconds before stalling.

I integrated your last experimental push on the repo. I also integrated the state machine file generation inside the STM32 VsCode workflow (generation of the HSM inside /Core/Lib)

The debug verbose option seems to be a very nice feature, it could be used to live debug embedded firmware directly on hardware. I tried to integrate the debbuger program with the putty serial output via a logfile but there is a strange format problem. Maybe the best option could be integrating the serial port listening inside hsmdebugger.py but it's not mandatory for the moment.

igor-krechetov commented 2 years ago

@Germwalker , yeah, the issue happened due to incorrect synchronization between tasks and ISR. Basically HSM was blocking ISR if there was a task trying to make a transition. And blocking ISR should never happen.

Please try the latest commit. I changed synchronization logic for ISR. Currently it will work only for a single core version of FreeRTOS. Do you need support for multi core too?

Also I added support for timers to the dispatcher and updated the example. (only state machine file was changed)

Germwalker commented 2 years ago

Heya @igor-krechetov,

Your last push indeed solved the collision between tasks & ISR, thank you very much.

We now face another problem (could be the last one) that cannot be reproduced on a POSIX system as it involve the memory management architecture of FREERTOS on embedded systems.

As we use heap3 (or heap4), a wrap of malloc() is provided as pvPortMalloc(). So when something allocates memory, it should call pvPortMalloc() to fill up the FreeRTOS heap region.

I tought heap3 allowed malloc() to be wrapped around pvPortMalloc() by default but it's not working this way : containers inside std:: as for example std::list calls the ARM toolchain malloc() when in need of memory and so the wrong heap is being filled up.

As defined in the xx_FLASH.ld file, the base heap is small :

/* Generate a link error if heap and stack don't fit into RAM */
_Min_Heap_Size = 0x200; /* required amount of heap  */
_Min_Stack_Size = 0x400; /* required amount of stack */

FreeRTOS overwrite this allocation with it's own stack & heap allocated sidely with the base heap. That's why even with a 50kB FreeRTOS heap size (as you can see 42kB is available) i have a __throw_bad_alloc() exception after running (crash around 60s) the double led toggling HSM :

hsmcpp-debug-01

Here is the call stack :

_exit@0x08028eb0 (Source inconnue:0)
abort@0x0802842e (Source inconnue:0)
std::__throw_bad_alloc()@0x08027fe2 (Source inconnue:0)
__gnu_cxx::new_allocator<hsmcpp::Variant>::allocate@0x08011aca (arm-none-eabi\include\c++\10.3.1\ext\new_allocator.h:112)
std::allocator_traits<std::allocator<hsmcpp::Variant> >::allocate@0x08011782 (arm-none-eabi\include\c++\10.3.1\bits\alloc_traits.h:460)
std::_Vector_base<hsmcpp::Variant, std::allocator<hsmcpp::Variant> >::_M_allocate@0x080112b2 (arm-none-eabi\include\c++\10.3.1\bits\stl_vector.h:346)
std::vector<hsmcpp::Variant, std::allocator<hsmcpp::Variant> >::_M_allocate_and_copy<__gnu_cxx::__normal_iterator<hsmcpp::Variant const*, std::vector<hsmcpp::Variant, std::allocator<hsmcpp::Variant> > > >@0x08013bde (arm-none-eabi\include\c++\10.3.1\bits\stl_vector.h:1511)
std::vector<hsmcpp::Variant, std::allocator<hsmcpp::Variant> >::operator=@0x0801311c (arm-none-eabi\include\c++\10.3.1\bits\vector.tcc:226)
hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::PendingEventInfo::operator=@0x08012984 (f7-hsmcpp-freertos\Core\Lib\hsmcpp\include\hsm.hpp:199)
hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::dispatchEvents@0x08012a2c (f7-hsmcpp-freertos\Core\Lib\hsmcpp\include\hsm.hpp:1139)
std::__invoke_impl<void, void (hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::*&)(), hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>*&>@0x0801c72a (arm-none-eabi\include\c++\10.3.1\bits\invoke.h:73)
std::__invoke<void (hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::*&)(), hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>*&>@0x0801b846 (arm-none-eabi\include\c++\10.3.1\bits\invoke.h:95)
std::_Bind<void (hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::*(hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>*))()>::__call<void, , 0u>(std::tuple<>&&, std::_Index_tuple<0u>)@0x0801a0e6 (arm-none-eabi\include\c++\10.3.1\functional:416)
std::_Bind<void (hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::*(hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>*))()>::operator()<, void>()@0x080186e8 (arm-none-eabi\include\c++\10.3.1\functional:499)
std::__invoke_impl<void, std::_Bind<void (hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::*(hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>*))()>&>(std::__invoke_other, std::_Bind<void (hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::*(hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>*))()>&)@0x08016bdc (arm-none-eabi\include\c++\10.3.1\bits\invoke.h:60)
std::__invoke_r<void, std::_Bind<void (hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::*(hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>*))()>&>(std::_Bind<void (hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::*(hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>*))()>&)@0x08015100 (arm-none-eabi\include\c++\10.3.1\bits\invoke.h:153)
std::_Function_handler<void (), std::_Bind<void (hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>::*(hsmcpp::HierarchicalStateMachine<BlinkMachineStates, BlinkMachineEvents>*))()> >::_M_invoke(std::_Any_data const&)@0x080134b4 (arm-none-eabi\include\c++\10.3.1\bits\std_function.h:291)
std::function<void ()>::operator()() const@0x0800a8fc (arm-none-eabi\include\c++\10.3.1\bits\std_function.h:622)
hsmcpp::HsmEventDispatcherBase::dispatchPendingEvents@0x0800a124 (f7-hsmcpp-freertos\Core\Lib\hsmcpp\HsmEventDispatcherBase.cpp:248)
hsmcpp::HsmEventDispatcherFreeRTOS::doDispatching@0x0800e032 (f7-hsmcpp-freertos\Core\Lib\hsmcpp\HsmEventDispatcherFreeRTOS.cpp:256)

So the std::allocator would be missing memory because it calls malloc() that fill base heap.

Two options are available :

The best solution should be doing both, prealocating inside FreeRTOS heap with a pvPortMalloc() wrapper. Some embedded engineers don't like calling new() during runtime, personally i don't care as long as the heap size is beeing contained and will not overflow.

The second solution can be implemented this way and work straightfully :

void *operator new(size_t size)
{
    return pvPortMalloc(size);
}

I tried to implement this new() wrapper, it's working outside ISR. Inside ISR i fall on this assertion inside port.c :

void vPortEnterCritical( void )
{
    portDISABLE_INTERRUPTS();
    uxCriticalNesting++;

    /* This is not the interrupt safe version of the enter critical function so
    assert() if it is being called from an interrupt context.  Only API
    functions that end in "FromISR" can be used in an interrupt.  Only assert if
    the critical nesting count is 1 to protect against recursive calls if the
    assert function also uses a critical section. */
    if( uxCriticalNesting == 1 )
    {
        configASSERT( ( portNVIC_INT_CTRL_REG & portVECTACTIVE_MASK ) == 0 );
    }
}

It's indeed forbidden to call a memory allocation inside an ISR. That's why prealocating the containers inside the HSM instance with the new() wrapper could be a good idea as it solves two problems at once and is efficient.

This page of the FreeRTOS documentation say something about malloc() inside ISR.

The hsm.hpp file uses std::list to dispatch so maybe lists can be preallocated, this stackoverflow talks about it :

preallocate list elements by inserting them into a "spare" list. Instead of inserting new elements, use the splice() method to move them from the "spare" list to the "primary" list. Instead of erasing elements, use the splice() method to move them from the "primary" list to the "spare" list.

This article from embedded artistry talks about std::list efficiency on Cortex-M CPUs. Seems pretty good, there is any L1 :

Many embedded systems do not utilize the cache, so in those situations std::list is still an acceptable choice. If you are relying on caching, consider std::vector instead of std::list.

About the multicores CPU support, as for me it's better to execute the hsm on a single CPU for the real time determinism. Some chips provides two Cortex-M4/M7 like the H7 family, but it would be uncomon to run a hsm on both cores, they are often used independently. ESP32 also uses a double core architecture but it's not real time capable.

There is a FreeRTOS API call you implemented inside the timer : uxTimerGetReloadMode() that is missing from the 1.2.1 version of the CubeMX FreeRTOS generated version only for STM32F7, but ST should update the firmware soon.

I'll post more about testing timers on Cortex-M4.

igor-krechetov commented 2 years ago

@Germwalker , unfortunately I don't see a way to fully avoid memory allocation inside of ISR. The main difficulty would be hsm::transition() implementation because internally it's using dynamic items. Even though it's possible to implement a custom container with cache support (out of the box, std::vector is not suitable for event queue logic due to constant necessity to remove first element which will result in a heavy memory copy) there are still 2 unsolvable issues:

So I see a couple possible options here:

  1. implement ISR safe heap_x.c based on existing one (I would try with #3 or 4). Modifying code like this seems to be enough:
    
    BaseType_t isInsideISR = xPortIsInsideInterrupt();

if (pdFALSE == isInsideISR) { vTaskSuspendAll(); } ... if (pdFALSE == isInsideISR) { ( void ) xTaskResumeAll(); }

2. Just not call transitions directly from ISR. Instead FreeRTOS APIs can be used to delegate **hsm::transition** calls to a task.
3. I have an idea for ISR safe version of the **hsm::transition()**, but it will have some limitations:
-- it will not support passing custom data arguments to transition()
-- it will have to use cache (so overall memory usage will increase) and transitions can be skipped if cache will get full (return value can be provided to identify the situation).

Please try option #1 (since that would be ideal if it works). If it will not solve the issue then please let me know what you think about #2 or #3.

As for uxTimerGetReloadMode(). I used it more for optimization. If it's not available it's ok to simply do:
```c++
vTimerSetReloadMode(timer, reloadMode);

instead of:

if (uxTimerGetReloadMode(timer) != reloadMode)
{
    vTimerSetReloadMode(timer, reloadMode);
}
igor-krechetov commented 2 years ago

@Germwalker , I implemented solution #3. Please try new API for interrupts: transitionInterruptSafe()

Germwalker commented 2 years ago

Hello @igor-krechetov ,

Your new API function solves the problem we faced with dynamic memory allocation inside ISR.

My example has been running nicely for a couple of hours now :

I tried to run the blue led with hsm timer with this harel's statechart : 12

I can't make the timers to work, neither the 08_freertos.scxml example. Is my statechart implementation of the timer right ? I tried to implement the same setup you used with the example but i can't see the led toggling every 1000 ms.

I had to make this cast to allow the compilation :

volatile int make_variant[] = {0, (vList.push_back(Variant::make(std::forward<Args>(args))), 0)...};
volatile int make_variant[] = {0, (vList.push_back(Variant::make((uint32_t)std::forward<Args>(args))), 0)...};

About the ISR memory allocation :

During the last few days i tried to reemplement the lists used inside the hsm with the etl::list library that allow a custom allocation at declaration to avoid allocator inside ISR.

What i understand with the new API implementation is that events generated from ISR are cached inside a vector then the dispatcher treat the events at the next freertos tick.

This is indeed much more efficient as running the hsm machinery inside an ISR. The only downside is the creation of a delay as ticks are executing at 1kHz, but it's not a problem as the hsm should not be hard real time but soft real time : event are computed with a deterministic deadline.

The best architecture to implement hsmcpp inside a real time application could be (top to bottom) :

In this context the hsm.transitionInterruptSafe() API enqueue events at 10x to 100x the evolving speed of the hsm.

This is of course application specific and doesn't need to be handled by the HSM.

Germwalker commented 2 years ago

Hello @igor-krechetov ,

I can confirm the hsm is running well on Cortex-M4/7 STM32 MCUs for an extended period of time. I didn't saw any crash in a week.

The last cloud is the timer but i guess i'm implementing it wrong ! I'll try to test it more in the next few days.

I'll also test the hsm on Cortex-M0 in few weeks.

igor-krechetov commented 2 years ago

@Germwalker , sorry for late reply.

Your state machine with timers looks fine. Maybe timers API work a bit differently on the MCU than in POSIX simulation.. Can you make a simple app that uses timers and works well on your MCU? (please use start, stop, restart operations) I then can use it as a reference to check FreeRtosDispatcher implementation.

As for execution delay.. Even if transitions are done only from Tasks, actual transition processing is up to your priority configuration for the dispatcher. Though I totally agree that in case of ISR one additional context switch will be required, causing transition to be a bit delayed.

And thank you for extensive testing. I'll merge FreeRTOS support to the main branch after we'll figure out how to fix timers.

Germwalker commented 2 years ago

Hello @igor-krechetov,

I found the problem ! It's related to the timer args validation introspection in the hsm.hpp file :

switch (action) {
        case StateAction::START_TIMER:
            argsValid = (newAction.actionArgs.size() == 3) && // -> return true
                         newAction.actionArgs[0].isNumeric() && // -> return true
                         newAction.actionArgs[1].isNumeric() && // -> return true
                         newAction.actionArgs[2].isBool(); // -> return false
            break;

If i force argsValid to true, the timer API is working and i can see the blue led blinking so the FreeRtosDispatcher timer implementation is not faulty.

Here are my timer loading functions :

registerTimer(static_cast<int>(BlinkMachineTimers::FTIM1), BlinkMachineEvents::ON_TIMER_FTIM1);
registerStateAction(BlinkMachineStates::OnBlue, StateActionTrigger::ON_STATE_ENTRY, StateAction::START_TIMER, static_cast<int>(BlinkMachineTimers::FTIM1), 50, true);
registerStateAction(BlinkMachineStates::OffBlue, StateActionTrigger::ON_STATE_ENTRY, StateAction::START_TIMER, static_cast<int>(BlinkMachineTimers::FTIM1), 50, true);

I figured out that the value of newAction.actionArgs[2] was 1 and not true, even if given true.

The variant is created here from your custom implementation :

makeVariantList(newAction.actionArgs, args...);

So it's related to the cast i have made here to be able to compile :

template <typename HsmStateEnum, typename HsmEventEnum>
template <typename... Args>
void HierarchicalStateMachine<HsmStateEnum, HsmEventEnum>::makeVariantList(VariantVector_t& vList, Args&&... args)
{
    volatile int make_variant[] = {0, (vList.push_back(Variant::make(**(uint32_t)**std::forward<Args>(args))), 0)...};
    (void)make_variant;
}

If i remove the cast the compiler yell like this :

Core/Lib/hsmcpp/inc/hsm.hpp:1172:69: error: call of overloaded 'make(int&)' is ambiguous
 1172 |     volatile int make_variant[] = {0, (vList.push_back(Variant::make(std::forward<Args>(args))), 0)...};
      |                                                        ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from Core/Lib/hsmcpp/inc/hsm.hpp:21,
                 from Core/Lib/blinkmachine/BlinkMachineBase.hpp:6,
                 from Core/Lib/blinkmachine/BlinkMachineBase.cpp:3:
Core/Lib/hsmcpp/include/variant.hpp:72:20: note: candidate: 'static hsmcpp::Variant hsmcpp::Variant::make(int8_t)'
   72 |     static Variant make(const int8_t v);
      |                    ^~~~
Core/Lib/hsmcpp/include/variant.hpp:73:20: note: candidate: 'static hsmcpp::Variant hsmcpp::Variant::make(int16_t)'
   73 |     static Variant make(const int16_t v);
      |                    ^~~~
[etc ...]

I think the cast i've made broke the args validation of the timer API as the bool parameter is now a uint32_t.

How can we resolve the ambiguous call ?

Germwalker commented 2 years ago

Hello @igor-krechetov ,

Here is something that resolve the ambigous call :

volatile int make_variant[] = {0, (vList.push_back(Variant::make((VariantVector_t)std::forward<Args>(args))), 0)...};

But a call to this test still return false :

bool test = newAction.actionArgs[2].isBool();
igor-krechetov commented 2 years ago

@Germwalker , seems like ambiguous call was caused because timer ID was passed as an int (which was ok for PC build, but was somehow ambiguous for your FreeRTOS build). I made a fix to explicitly use int32_t, but you'll need to re-generate BlinkMachineBase file again.

Also, since your test results look very promising, I integrated FreeRTOS support to main branch. Please use it for your build.

And please let me know if you had to do any custom modifications to the code (hsm or generated one) to make it compilable for your FreeRTOS build.

Please let me know if everything is working ok for you so I can close this issue. :)

Germwalker commented 2 years ago

Hello @igor-krechetov ,

I integrated the main branch inside the STM32 workflow.

I still have to force this cast to allow the compilation (error: call of overloaded 'make(int&)' is ambiguous) :

template <typename HsmStateEnum, typename HsmEventEnum>
template <typename... Args>
void HierarchicalStateMachine<HsmStateEnum, HsmEventEnum>::makeVariantList(VariantVector_t& vList, Args&&... args)
{
    volatile int make_variant[] = {0, (vList.push_back(Variant::make((uint32_t)std::forward<Args>(args))), 0)...};
    (void)make_variant;
} 

I regenerated the state machine files.

I also had to force the timer args validation to make timers to work, this way :

switch (action) {
        case StateAction::START_TIMER:
            argsValid = (newAction.actionArgs.size() == 3) &&
                         newAction.actionArgs[0].isNumeric() &&
                         newAction.actionArgs[1].isNumeric() &&
                         newAction.actionArgs[2].isBool();
            argsValid = true;
         break;

I also tested this after the switch :

bool test = newAction.actionArgs[2].isBool(); -> false
bool test1 = newAction.actionArgs[2]; -> true
uint32_t test2 = newAction.actionArgs[2];  -> 1

This is suspicious, i also suspect this parameter to mess with the timer type (repeat, single shot) because if i call a single shot timer this way :

registerTimer(static_cast<int>(StationMachineTimers::FTIM), StationMachineEvents::ON_TIMER_FTIM);
registerStateAction(StationMachineStates::STATE1, StateActionTrigger::ON_STATE_ENTRY, StateAction::START_TIMER, static_cast<int>(StationMachineTimers::FTIM), 5000, true);

Then the log show this during runtime :

[HSM_BASE] -> startTimer : [DEBUG] handlerID=2, timerID=0, intervalMs=5000, isSingleShot=0
[HSM_RTOS] -> startTimerImpl : [DEBUG] timerID=0, intervalMs=5000, isSingleShot=0

As i understand the log it's now a repeated timer. That's why i'm wondering about the integrity of the boolean parameter inside the registerTimer() API.


Thank you for the merge ! HSMCPP is now functionnal on MCU and it should be compatible with all non posix targets that have a freertos port.

I will bother you a bit more soon with some questions on the statechart transcompilation and intepretation but i'll do it inside a new issue and after extensive testing.

I plan to use and support this project on the long term as it adress all my needs on embedded computing. Many thanks for the flawless support during this past two months, i've learn alot about statecharts and C++ !

Germwalker commented 2 years ago

Hello @igor-krechetov ,

I found a small mistake in the timer launching procedure :

if (StateAction::START_TIMER == actionInfo.action)
{
    mDispatcher->startTimer(mTimerHandlerId,
                            actionInfo.actionArgs[0].toInt64(),
                            actionInfo.actionArgs[1].toInt64(),
                            actionInfo.actionArgs[2].isBool());

should be :

if (StateAction::START_TIMER == actionInfo.action)
{
    mDispatcher->startTimer(mTimerHandlerId,
                            actionInfo.actionArgs[0].toInt64(),
                            actionInfo.actionArgs[1].toInt64(),
                            actionInfo.actionArgs[2].toBool()); // or just actionInfo.actionArgs[2]

The timer was always launched as repeated even if given isSingleShot=true because isBool() always return false.

I also tested this in the same if () test :

bool test6 = actionInfo.actionArgs[2].toBool(); -> return false
bool test7 = actionInfo.actionArgs[2]; -> return true

I don't know why but this conversion seems to be broken on Cortex-M as it always returns false :

bool Variant::toBool() const
{
    bool result = false;

    if (Type::BOOL == type)
    {
        result = *value<bool>(); -> never get here
    }

    return result;
}

I tested the value of type and it's always hsmcpp::Variant::Type::UBYTE_4.

Seems to be a 32 bits value :

Variant Variant::make(const uint32_t v) { return Variant(new uint32_t(v), Type::UBYTE_4); }

Even inside isBool(), the type is hsmcpp::Variant::Type::UBYTE_4 :

bool Variant::isBool() const
{
    return type == Type::BOOL;
}

Maybe bool type is not represented the same way on a posix system than inside Cortex-M ?

igor-krechetov commented 2 years ago

@Germwalker , it looks like whatever compiler you are using is treating bool as int... That would make sense it you would use C compiler, but cpp files should be automatically compiled by a C++ compatible compiler... Anyway, I modified variant class to be more flexible about boolean type (latest version 0.24.2). Now isBool/toBool should work correctly on your platform. (also there is a unit-test in tests/20_variant.cpp : type_boolean . Please try running this logic if you still have issue)

I couldn't find this code in hsmcpp, even in older versions. It always was "actionInfo.actionArgs[2].toBool());" . Can you tell me in which revision you found this?

if (StateAction::START_TIMER == actionInfo.action)
{
    mDispatcher->startTimer(mTimerHandlerId,
                            actionInfo.actionArgs[0].toInt64(),
                            actionInfo.actionArgs[1].toInt64(),
                            actionInfo.actionArgs[2].isBool());

Also I noticed that you didn't use latest generator. Your repository currently contains this:

registerStateAction(BlinkMachineStates::OnBlue, StateActionTrigger::ON_STATE_ENTRY, StateAction::START_TIMER, static_cast<int>(BlinkMachineTimers::FTIM1), 500, true);

When it should be (_static_cast_):

registerStateAction(BlinkMachineStates::OnBlue, StateActionTrigger::ON_STATE_ENTRY, StateAction::START_TIMER, static_cast<TimerID_t>(BlinkMachineTimers::FTIM1), 500, true);

Please let me know if this solved your issues with hsmcpp. :)

Germwalker commented 2 years ago

Hello @igor-krechetov ,

I made two mistakes in my previous post :

I corrected this and also integrated the lastest variant.cpp file, deleting my enforcing stuff.

I noticed something and also changed my blinking machine architecture accordingly : i had a memory leak issue while using the API function _starttimer(TIM, time, TRUE); between two sub states. It's a better idea to start the repeating timer in the top state onEntry() then use the _ON_TIMERTIM call to make transitions, no more leak.

Everything is working fine ! The issue can now be closed. I'll let you know if i found other issues on the larger scale project i'm working on.