Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup of fixedwing main using new sys_time timers #123

Merged
merged 5 commits into from Feb 8, 2012
Merged

Conversation

flixr
Copy link
Member

@flixr flixr commented Jan 30, 2012

@gautierhattenberger @dewagter
I started cleaning up main using the new sys_time timers to get rid of the all the prescaling for the different periodic functions.
What do you think about this?

@gautierhattenberger
Copy link
Member

Why not leaving the ap tasks in main_ap (same for fbw) ?
With this way, we are not sure that the sequence sensor/navigation/control is kept at every cycle, right ? It is probably fast enough so we won't see a difference, but are we sure there are no border effects ?

@flixr
Copy link
Member Author

flixr commented Jan 31, 2012

Why not leaving the ap tasks in main_ap (same for fbw) ?

What do you mean by leaving them in main_ap? If we use the timers, they should be checked from the main loop. We could check them from the ap/fbw event loops though (which would amount to the same thing). It would probably be nicer to read?

With this way, we are not sure that the sequence sensor/navigation/control is kept at every cycle, right?

Correct. But I'm not convinced this is an issue with this firmware. By default sensors runs at 60Hz (for IMU most run it a lot higher), navigation was only run at 4Hz (can easily up that to something higher now if you have a faster gps), control was run at 20Hz by default so far.
So the only thing that I could think of is that you might have a delay between sensors and control of 1/PERIODIC_FREQUENCY in the worst case.
We can also put sensors/control both into periodic_task_ap ... Although that does get uglier for higher sensor rates again.

I started this, since it is easier to discuss these things if you can take a look at the code instead of only talking about it...
Also I would really like to use an RTOS scheduler to run the different tasks (especially estimation that might take longer) in the future. So you can probably consider this a start to make it easier to integrate (and turn on/off) the scheduler later.

@gautierhattenberger
Copy link
Member

I think main.c should stay like it was, and call specific code in main_ap or main_fbw

@flixr
Copy link
Member Author

flixr commented Jan 31, 2012

If I understand you correctly I probably agree:
The change then would be to move the timer checks and task calls to the event_task_ap/fbw. It would be functionally exactly the same, just nicer to read and things are were they belong.
But then the questions is why keep a call to periodic in main.c at all? We could just call event_task_fbw and event_task_ap from main, and run all "tasks" (including perdiodic_task_x) from the event loop checking the timers in the same way as I quickly wrote into main.c.
Maybe even get rid of periodic_task_ap and just add one for the modules (currently nothing else left in there anyway)? But this depends on if we really want to separate sensors and control like I did here (mainly for illustrative purposes) so far.

@gautierhattenberger
Copy link
Member

Having no periodic calls in main could be an option here, I agree.
For the current firmware, the registering and calling of periodic tasks can be done by hand. Moreover, there should not be any implementation of tasks such as navigation_task or reporting_task in main_*. These two files should be only dedicated to scheduling. So, it becomes to replace them by some RTOS in the future.

@flixr
Copy link
Member Author

flixr commented Jan 31, 2012

Yes, exactly. I just didn't start re-factoring the nav stuff out yet, that needs a lot more cleanup. Same goes for the attitude loop. For the reporting_task it would be easy.
Also when we start refactoring things like navigation/control task for fixedwings, we should probably have an eye towards rotorcraft. Ideally the interfaces for the "tasks" would be the same...

I know this all raises a lot of questions about the overall design and on how to restructure things now and we could go on:
Stuff like datalink could probably become a module? Enhance subsystems to have similar xml definitions like the modules with event functions, valid parameters, etc. (as was already discussed several times)...

But since we need to do this step by step, I just started and the basic question I see here now (and for the future) is whether separating sensors and control like this would lead to any problems (delay too high?). I also forgot that while control defaults to 20Hz it is normally run at 60Hz (defined in most airframe files).

I also "rewrote" the rotorcraft main with the timers (see cbe56b9 in systime branch). But there we don't have an ap/fbw split and sensors/control are run together at the highest rate.

… timers for periodic tasks from there

also some cosmetics for main_ap.c
* removed obsolete/unused: fatal_error_nb, ac_ident and INIT_MSG_NB
* moved rc_settings_mode, slider_1_val and slider_2_val to rc_settings
* cosmetics
@flixr
Copy link
Member Author

flixr commented Feb 1, 2012

@gautierhattenberger @dewagter

If this principle is acceptable to you, I will merge this into dev. We can then start further cleaning up stuff like the reporting_task, nav, etc.
Also the PERIODIC_FREQUENCY / AHRS_PROPAGATE_FREQUENCY lowpassing stuff needs cleanup.

Regarding the sequence of sensors/navigation/control, IMHO this is currently a non-issue:

  • Navigation is run at 4Hz only anyway.
  • If using IR sensors, ahrs_update_infrared() is called at the beginning of attitude_loop(), so no change here...
    • But maybe this should be done in sensor_task() as well? (just uses the latest available ir measurements to update the estimated attitude)
  • If using an imu: imu_periodic() is run in the sensors_task(), but this just schedules a new read.
    • the ahrs is only updated when measurements are actually available: on_gyro_event()

So if this wasn't an issue so far, it won't be an issue now...

@gautierhattenberger
Copy link
Member

As you said, it needs more cleaning, but for me looks good enough to go to dev

flixr added a commit that referenced this pull request Feb 8, 2012
cleanup of fixedwing main using new sys_time timers
@flixr flixr merged commit 15bee52 into systime Feb 8, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants