Cicada-FW icon indicating copy to clipboard operation
Cicada-FW copied to clipboard

Zephyr porting and organizational questions

Open martinjaeger opened this issue 5 years ago • 3 comments

Hey. First of all thanks for open sourcing your libraries.

We are currently trying to port it to Zephyr RTOS and encountered a few questions.

Here is the current status of the port: https://github.com/LibreSolar/cicada-library/commit/8c52b9d9c0a8f28a2145aea5b082a3520c80ed1f

  1. Which is the repository where ongoing development will happen and where I should send a PR to? I see that new updates are pushed to okrasolar/Cicada, but that one does not allow to submit issues. That's why I'm posting here.

  2. The library is compiling and the SIM800 module is blinking, but the thread it runs in gets stuck waiting for !commDev.isConnected(). As it is cooperative (which is required according to some comments in the code), nothing else is happening anymore in the charge controller either.

  3. How do you suggest to trace down issues? I added a global CICADA_DEBUG define already, but it doesn't print any additional output so far. Is there any other trick?

It would be great if you could have a quick look at the initialization of all the different components if they look fine: https://github.com/LibreSolar/charge-controller-firmware/blob/cicada/src/ext/modem.cpp

martinjaeger avatar Mar 11 '20 15:03 martinjaeger

Hey Martin!

1. Which is the repository where ongoing development will happen and where I should send a PR to? I see that new updates are pushed to [okrasolar/Cicada](https://github.com/okrasolar/Cicada), but that one does not allow to submit issues. That's why I'm posting here.

Yeah, okrasolar/Cicada is the correct one. I don't know why it had issues disabled, I enabled it now.

2. The library is compiling and the SIM800 module is blinking, but the thread it runs in gets stuck waiting for `!commDev.isConnected()`. As it is cooperative (which is required according to some comments in the code), nothing else is happening anymore in the charge controller either.

You must make sure the driver's run() function is called in the correct intervals. You could either call it directly from your yieldFunction(), or when you use a cooperative scheduler create another thread and call it form there. Easiest is probably to call it from the yieldFunction(), something like:

void yieldFunction(void *parameter)
{
    Task* t = (Task*)parameter;
    k_sleep(t->delay());
    t->run();
}

If you have multiple Cicada tasks to call (probably not the case in your scenario), it also provides it's own basic scheduler for that.

3. How do you suggest to trace down issues? I added a global `CICADA_DEBUG` define already, but it doesn't print any additional output so far. Is there any other trick?

All actual stuff happens in the driver's run() function, so if you didn't call that, there probably wasn't any debug output to see. Also, Cicada uses the embedded printf for it's debug output, make sure this is setup correctly. Especially, it requires a _putchar() function, which forward the characters to your debugging UART.

lippit avatar Mar 12 '20 10:03 lippit

Hi Georg, thanks for the quick answer.

So I will use the other repo for issues from now on but continue here with this one.

Being more familiar with C, I'm getting a bit lost in the C++ interfaces and classes and I'm not totally sure which ones I actually need to use and which parts I can use from the Zephyr RTOS. Sorry if my questions sound stupid.

Given that we have an RTOS already, I thought I don't need an extra scheduler or task class and I can just run all modem stuff in a dedicated Zephyr thread. Sounds like I still need the Task class, but can use the Zephyr scheduler, correct? But why does the mbed example use the Cicada Scheduler and not its built-in one?

Which would be the correct interval the run() function needs to be called? Zephyr has a kernel timer function that can call any function in a regular interval. But the function runs in an interrupt context, so it needs to be short. It could e.g. wake up the cicada thread again. Does that make sense?

Re printf: Zephyr has a globally available printf function already. Do I still need to use the mentioned library?

martinjaeger avatar Mar 12 '20 17:03 martinjaeger

Hey Martin!

Given that we have an RTOS already, I thought I don't need an extra scheduler or task class and I can just run all modem stuff in a dedicated Zephyr thread. Sounds like I still need the Task class, but can use the Zephyr scheduler, correct? But why does the mbed example use the Cicada Scheduler and not its built-in one?

The reason behind all this is that Cicada was designed to also run on bare metal even without an OS. When it's used with an OS, there are multiple ways how to execute it's internal run() methods. Easiest is probably still to use the Cicada scheduler. Depending on how the OS works, it is probably a bit more efficient to use the OS scheduler though.

In any case, you need to make sure that:

  1. Every tasks run() function is called in time
  2. Every access to the Cicada functions is synchronized, meaning that you should not change a classes internal state from multiple threads (or even interrupts) at the same time.

Which would be the correct interval the run() function needs to be called? Zephyr has a kernel timer function that can call any function in a regular interval. But the function runs in an interrupt context, so it needs to be short. It could e.g. wake up the cicada thread again. Does that make sense?

It should not run in fixed intervals, but when it's due. This can be found with Task::delay(). As in my example before, what you can do is:

  1. Call run()
  2. Sleep for Task::delay()
  3. Go back to 1

You should definitely NOT call run() directly from an interrupt.

Re printf: Zephyr has a globally available printf function already. Do I still need to use the mentioned library?

I think using Zephyr's printf should be fine. Actually, the only place in the lib which currently does logging is simcommdevice.cpp, which includes printf.h. Just make sure it uses the correct printf in that file.

lippit avatar Mar 16 '20 09:03 lippit