CMSIS RTOS v1 Signals Implementation Issue

[anthonytex]

Hi Zephyr devs,

yesterday i've try zephyr for the first time.
My idea was to port a simple firmware that is written with CMSIS RTOS API v1 to Zephyr.
For this, i'm using zephyr's portability layer: CONFIG_CMSIS_RTOS_V1=y.

This firmware uses signals API to sync threads, but something is not working, here there is a simple program to test the issue:

#include <kernel.h>
#include <cmsis_os.h>

void print_thread(void const *args)
{
    printk("Started Thread 1, waiting for signals\n");

    while (true)
    {
        osSignalWait(0x1, osWaitForever);
        printk("Signal Received\n");
    }
}

osThreadDef(print_thread, osPriorityNormal, 1, 256);

int main(void)
{
    osThreadId tid = osThreadCreate(osThread(print_thread), NULL);
    //Send signals every second
    while (true)
    {
        printk("Waiting\n");
        osDelay(1000);
        osSignalSet(tid, 0x1);
    }
}

Digging in, i've found 2 problems:

1. when the thread is created:

   zephyr/lib/cmsis_rtos_v1/cmsis_thread.c

   Line 83 in e90873f

   k_thread_custom_data_set((void *)thread_def);

   
   the custom data that is used for signals handling in your CMSIS implementation is, for what i can see, set for the current running thread and not for the created new thread: so when you call

   zephyr/lib/cmsis_rtos_v1/cmsis_signal.c

   Line 90 in e90873f

   osThreadDef_t *thread_def = k_thread_custom_data_get();

   you are pointing to other thread data.
2. By assuming that the custom data was properly set, then when the thread is created by calling

   zephyr/lib/cmsis_rtos_v1/cmsis_thread.c

   Line 85 in e90873f

   tid = k_thread_create(&cm_thread[thread_def->instances],

   the custom data is reset by this

   zephyr/kernel/thread.c

   Line 568 in e90873f

   new_thread->custom_data = NULL;

So signals handling for me is not working, a simple hack fixes the problem (only for demostration, sorry but i've not a deep knowledge of zephyr so feel free to fix this in a better way of course):

cm_thread = thread_def->cm_thread;
	atomic_dec((atomic_t *)&thread_def->instances);
	stk_ptr = thread_def->stack_mem;
	prio = cmsis_to_zephyr_priority(thread_def->tpriority);
	//k_thread_custom_data_set((void *)thread_def);

	tid = k_thread_create(&cm_thread[thread_def->instances],
			stk_ptr[thread_def->instances], stacksz,
			(k_thread_entry_t)zephyr_thread_wrapper,
			(void *)arg, NULL, thread_def->pthread,
			prio, 0, K_NO_WAIT);
       tid->custom_data = (void *)thread_def;

Also your test suit does not cover this case, in fact the current thread is waiting to be signaled and not the newly created like in my example:

zephyr/tests/portability/cmsis_rtos_v1/src/signal.c

Lines 72 to 90 in e90873f

	void test_signal_events_no_wait(void)
	{
	osThreadId id1;
	osEvent evt;
	id1 = osThreadCreate(osThread(Thread_1), osThreadGetId());
	zassert_true(id1 != NULL, "Thread creation failed");
	/* Let id1 run to trigger SIGNAL1 */
	osDelay(10);
	/* wait for SIGNAL1. It should return immediately as it is
	* already triggered.
	*/
	evt = osSignalWait(SIGNAL1, 0);
	zassert_equal(evt.status, osEventSignal, "");
	zassert_equal((evt.value.signals & SIGNAL1), SIGNAL1, "");
	osThreadTerminate(id1);
	}

Thanks

[carlescufi]

@anthonytex thanks for the extensive analysis. Perhaps you'd like to send a Pull Request fixing the behavior?

[anthonytex]

Hi @carlescufi , i've just open a PR but for another issue. In these days i'm going to find some time to send a PR to fix this.
Thanks

[github-actions]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.