search

PX4 / PX4-user_guide

PX4 User Guide
https://docs.px4.io/main/en/index.html
Other
313 stars 1.65k forks source link

Add user guide for new ackermann module #3265

Closed chfriedrich98 closed 2 months ago

chfriedrich98 commented 3 months ago

This PR adds a new section to the Rovers tab on the new ackermann rover module

This is the start of an update on the whole rover (and later USV) documentation in PX4.

hamishwillee commented 3 months ago

@chfriedrich98 Appreciate that you're still draft, but I've done a subedit to the standard docs conventions, not all of which are documented in https://docs.px4.io/main/en/contribute/docs.html#style-guide . I also ran prettier.

When you move out of draft I'll give it a more thorough review. But on quick scan I think this looks excellent. About time we had some good docs for Rover.

A few things in passing - you cover flight modes here - we need to make this work with https://docs.px4.io/main/en/flight_modes_rover/

For mission mode it might be worth adding a dedicated page, but either way, would be good to list the commands that can be used in a mission, and how the system works with respect to when others are missing or present. For example, takeoff is a meaningless rover mission item, but it is also harmless. We should still make it clear whether that command would be ignored or fail a feasibility test.

We should also clarify what happens when the mission is paused (presumably vehicle just stops, but does it go into some kind of "hold mode" (undocumented) or is it in a paused mode "within mission mode".

When we get to boat we'll need to cover hold behaviour too - vehicles might follow a radius like a fixed wing, or they might head to centre of a hold circle and drift, returning to centre on exiting the radius, or something else.

Thanks again.

hamishwillee commented 3 months ago

PS I assume this is in v1.15? In that case we will need a release note in https://docs.px4.io/main/en/releases/1.15.html

hamishwillee commented 3 months ago

(That could go in before the rest of this).

chfriedrich98 commented 3 months ago

@hamishwillee Thank you for the review! I'll start implementing your suggestions and re-request a review once I move out of draft.

PS I assume this is in v1.15? In that case we will need a release note in https://docs.px4.io/main/en/releases/1.15.html

Yes, I'll have a look and open a seperate PR for the release notes entry. Nevermind, it is planned vor 1.16.

hamishwillee commented 2 months ago

Nevermind, it is planned vor 1.16.

Great, then can you please, when you get to it, add a release note to the doc in en/releases/main.md

Note, I will force-rebase this next, to fix some conflicts with the updated sidebar.

chfriedrich98 commented 2 months ago

@hamishwillee I added the realease notes and made some minor improvements. In regards to the flight modes: With the restrucutre of the USV support the different types of rovers no longer share the same module so the supported flight modes are different and each would have their own definitions. The goal with the restructure is to eventually remove support for the old rover module (which was used for all USV's) and with it most of the current rover related doc entries, which would include https://docs.px4.io/main/en/flight_modes_rover/. Until then there is currently some conflicting information I tried to solve by adding this warning https://github.com/PX4/PX4-user_guide/pull/3265/files#diff-68836498fff5b72612f91d31bfa9e8c70a34bbed4bb0f9696e5816afe563f2c5R31-R34.

chfriedrich98 commented 2 months ago

This is the structure I have in mind:

USV (Experimental) (some basic introduction to USV support in PX4)

And the USV tab could be on the same hieararchy level as Multicopter/planes and vtol.

hamishwillee commented 2 months ago

@chfriedrich98 Thanks very much for the heads up. I'm now on another job until Wednesday, so I will look at this "properly" then.

What if anything is planned to be shared, if anything? What I'm heading towards here is that I would expect that USVs would share modes such as manual and mission - since the behaviours of all are likely to be the same in most cases. However there might be some exceptions between UGVs and boats - for example hold behaviour for a boat can vary quite a lot. Are you "really" planning on having completely separate implementations even for missions?

If the intent is that these will be very separate and continue to be very separate then I very much like your proposal above https://github.com/PX4/PX4-user_guide/pull/3265#issuecomment-2222725717 - and I think we should work to move there earlier rather than later.

So essentially, my plan would be to move to your proposed structure now, but initially keep all of that down in experimental section until one of the USVs becomes "non-experimental". Does that make sense, or do you feel it is too early?

chfriedrich98 commented 2 months ago

@hamishwillee

What if anything is planned to be shared, if anything? What I'm heading towards here is that I would expect that USVs would share modes such as manual and mission - since the behaviours of all are likely to be the same in most cases. However there might be some exceptions between UGVs and boats - for example hold behaviour for a boat can vary quite a lot. Are you "really" planning on having completely separate implementations even for missions?

Manuall/Mission are going to be very similar for all USVs , so I think it could make sense to have a USV Flight Modes section and mention there which USVs support which flight modes (and of course also in the respective USV docs). @PerFrivik wrote a very nice flight modes documentation in #3133 and I would propose to use a simplified version of this that applies to all USVs in the general USV Flight modes tab and then expand on it in the individual modules. I think that would make sense since the very detailed explanation of the individual guidance algorithms is not necessary for the user to understand to use it.

So essentially, my plan would be to move to your proposed structure now, but initially keep all of that down in experimental section until one of the USVs becomes "non-experimental". Does that make sense, or do you feel it is too early?

I think this PR could be merged on its own for now, since it is esentially a standalone file. If we do the same for differential rover we would then have all the "pieces" ready for the restructure in a subsequent PR (this could be sinchronized with the PX4 PR that removes the old rover module). During this transition we just need to clearly indicate that there is currently 2 different ways to run rovers. Keeping the USVs in the experimental tab until they are more established makes sense to me.

hamishwillee commented 2 months ago

Some of the documented params don't exist - probably changed names after you wrote this. YOu can see them listed by the flaw checker https://github.com/PX4/PX4-user_guide/pull/3265#issuecomment-2235743284

I fixed the ones I could be sure of, but you'll have to update the docs for the others appropriately.

chfriedrich98 commented 2 months ago

@hamishwillee Thanks for the detailed review! I'll start implementing your suggestions and ping you again once I'm through.

Some of the documented params don't exist - probably changed names after you wrote this. YOu can see them listed by the flaw checker https://github.com/PX4/PX4-user_guide/pull/3265#issuecomment-2235743284 I fixed the ones I could be sure of, but you'll have to update the docs for the others appropriately.

Yes I think i renamed some of them because the names were too long, I'll get right on that!

github-actions[bot] commented 2 months ago

No flaws found

hamishwillee commented 2 months ago

@chfriedrich98 Thanks very much for all the updates.

OK, so I have "positioned" this module now. Essentially renamed the old and new modules to "Ackermann Rover (v1)" and "Ackermann Rover (v2)". I've split out the docs for the "v1" and "differential steering" so that there are three very clear choices. I moved the flight mode stuff into the v1 and diff steering docs because they are not the same as the ackermann v2.

I think we're good with this because it is pretty clear what differs and what your choices are. I decided not to split out the mission mode for now though it is a bit messy, we can do so easily enough as this progresses.

I haven't merged, because I'd appreciate you checking that what I have done makes sense. We can merge tomorrow if it is.

BTW, will the new module accept offboard commands yet?

chfriedrich98 commented 2 months ago

@hamishwillee

I haven't merged, because I'd appreciate you checking that what I have done makes sense.

I think this works very well for the transition period. I have taken over the development of the differential rover, so once I get those updates merged upstream I'll create a new docs PR in which I'll add an updated section for differential and also finalize the restructure we discussed.

BTW, will the new module accept offboard commands yet?

I have not done any testing with offboard control yet, so I would not say that it properly supports it.

hamishwillee commented 2 months ago

Excellent. So pleased you're taking over the differential stuff too! Merging - we can iterate.

hamishwillee commented 2 months ago

@chfriedrich98 FYI have posted about this this on #rover channel in discord - https://discord.com/channels/1022170275984457759/1026905519560077372/threads/1265812295804522557

I couldn't tag you because I don't know your ID.

hamishwillee commented 2 months ago

PS How are we planning on deploying this "eventually"? Is it that we'll dump the old module and we'll have space in the standard build? Or is it that we need some mechanism to allow firmware variants to be selected in QGC (my preference)? Do you know who is sorting this out?

chfriedrich98 commented 2 months ago

PS How are we planning on deploying this "eventually"? Is it that we'll dump the old module and we'll have space in the standard build? Or is it that we need some mechanism to allow firmware variants to be selected in QGC (my preference)?

Getting rid of the old module is unfortunately not enough since it is not even enabled on the default build for all boards. The second option would be ideal, but I'm not familiar with that topic so I don't know how difficult that change is.

Do you know who is sorting this out?

I don't know of anyone that is working on this.

chfriedrich98 commented 2 months ago

@hamishwillee Just FYI I started working on the restructure for the rover docs as we discussed here in https://github.com/PX4/PX4-user_guide/pull/3311. It is still a draft (some upstream PR's have to be merged first too) so there is no need to review it yet, but I set up the overall structure which I'd appreciate your input on.