Refactoring dataman to remove locking mechanisms and to use uORB #21594

Igor-Misic · 2023-05-15T14:58:49Z

Changes brought by this PR:

Refactored the initialization process of the Dataman file.
Removed the thread-locking mechanism from the Dataman thread.
Added two new classes, DatamanClient and DatmanCache, which facilitate communication with Dataman using uORB messages.
Removed the locking mechanism for Dataman itself

Refactored Initialization of the Dataman File
This fix addresses the issue outlined in the following link: #21278.
Now, if the file doesn't exist or contains an incorrect compact key, it will be cleared, and default values will be set.

DatamanClient
Introduces a new method of communicating with the Dataman thread. Previously, direct access to the Dataman module was used, where requests were inserted into the queue with a semaphore. This approach has now been replaced with the uORB messaging system.

DatamanClient introduces both synchronous and asynchronous modes of communication with Dataman.

DatamanCache
This class serves as a cache storage for a defined amount of Dataman items. Currently, the cache does not employ any advanced caching algorithms.

Integration within mission
The mission now incorporates a simple caching mechanism that attempts to cache the next N items whenever the "current item" is modified.
In order to eliminate the need for a locking mechanism within the mission, logic has been implemented to check if the set of mission items has been updated in the Dataman. The write operation to DM_KEY_MISSION_STATE has been removed from the mavlink thread, and instead, the existing uORB (mission.msg) message is utilized.

Integration geofence and safe points
As the information on new geofence and safe points is updated at the end, it is safe to utilize it without the need for a lock. Integration has been implemented to store all geofence and safe point items within the cache and update them whenever changes occur.

Overview of the new architecture

Stack is increased as it was needed

julianoes · 2023-05-30T05:14:29Z

Nice, this looks amazing @Igor-Misic!

bkueng

Nice work @Igor-Misic! I went through it, tested quite a bit, and fixed some things.

RAM usage increases a bit (depending on geofence/safe point usage ~2KB).

Navigator does not block anymore during a mission execution (unless DO_JUMP is used). This is in sih with geofence and safe points:

PR:
navigator: 2337 events, 121802us elapsed, 52.12us avg, min 21us max 803us 650.441us rms
main:
navigator: 2539 events, 919572us elapsed, 362.18us avg, min 19us max 26094us 3136.414us rms

dagar · 2023-06-16T14:16:47Z

.ci/Jenkinsfile-hardware

@@ -449,7 +449,7 @@ pipeline {
                    sh './Tools/HIL/run_nsh_cmd.py --device `find /dev/serial -name *usb-*` --cmd "uorb_tests latency_test" || true'

                    // test dataman
-                    sh './Tools/HIL/run_nsh_cmd.py --device `find /dev/serial -name *usb-*` --cmd "tests dataman"'
+                    sh './Tools/HIL/run_nsh_cmd.py --device `find /dev/serial -name *usb-*` --cmd "tests dataman" || true'


Igor explained it in the commit: 50f23de.

I made it a bit more explicit in b0c918f

dagar · 2023-06-16T15:01:19Z

There's a failure on the test rack that looks real. http://px4-jenkins.dagar.ca:8080/blue/organizations/jenkins/PX4-Autopilot/detail/pr-dataman_refactoring/21/pipeline/125

bkueng · 2023-06-19T09:59:22Z

There's a failure on the test rack that looks real. http://px4-jenkins.dagar.ca:8080/blue/organizations/jenkins/PX4-Autopilot/detail/pr-dataman_refactoring/21/pipeline/125

I didn't reproduce locally, but I think I fixed it in the last commit.

tstastny · 2023-06-19T12:01:53Z

looks like only remaining build failure is 0.01% flash on diatone mamba

tstastny · 2023-06-21T11:57:18Z

@dagar - what do you think? gtg?

dagar · 2023-06-21T17:43:24Z

src/modules/navigator/mission.cpp

@@ -527,6 +554,14 @@ Mission::update_mission()
 	const mission_s old_mission = _mission;

 	if (_mission_sub.copy(&_mission)) {
+
+		bool success = _dataman_client.writeSync(DM_KEY_MISSION_STATE, 0, reinterpret_cast<uint8_t *>(&_mission),


Why?
DM_KEY_MISSION_STATE and ORB_ID(mission) are the same thing, mavlink_mission would have published mission and written DM_KEY_MISSION_STATE back to back. I think it's time to kill DM_KEY_MISSION_STATE and remove some of the confusion.

dagar · 2023-06-21T17:51:35Z

DM_KEY_MISSION_STATE is obsolete and can be purged, we don't need to save the progress to the SD card
publishing ORB_ID(mission) is used to signal navigation mission to update and rerun mission feasibility, but now it's also updating with any geofence or safe point changes
DatamanClient is using the old uORB API where a subscription is a file descriptor, we need to check that the extra FD usage doesn't trigger the warning (or change to uORB::SubscriptionBlocking, etc)

A version update is needed since the dataman is showing errors if data doesn't exist or if it is wrongly stored. This will force default data to be initialized.

tests dataman will trigger errors and run_nsh_cmd.py is not written to detect only when a test fails. This is properly tested with run_tests.py

…anges This avoids the need to regularly access dataman for checking.

Avoids unnecessary dataman accesses

…dataman' As this test is expected to contain errors.

This can happen e.g. when the item is cleared.

Instead of also when geofence/safe points updated. This prevents reporting multiple times.

bkueng · 2023-07-24T11:10:22Z

@dagar I'm getting this in as a few things are blocking on this. I'll do a follow-up, if there's anything else needed.

DronecodeBot · 2023-09-15T11:33:28Z

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/how-to-start-developing/34131/4

Igor-Misic force-pushed the pr-dataman_refactoring branch 3 times, most recently from c326949 to 4c0713b Compare May 16, 2023 14:50

dagar self-requested a review May 17, 2023 01:07

Igor-Misic force-pushed the pr-dataman_refactoring branch 3 times, most recently from f8b538d to 56d0d30 Compare May 17, 2023 14:14

Igor-Misic added the Architecture label May 17, 2023

Igor-Misic force-pushed the pr-dataman_refactoring branch 6 times, most recently from e0dd2cc to 7768e5d Compare May 22, 2023 09:37

Igor-Misic marked this pull request as ready for review May 22, 2023 11:34

Igor-Misic requested a review from bkueng May 22, 2023 11:34

bkueng force-pushed the pr-dataman_refactoring branch 2 times, most recently from f2705c3 to 2f16266 Compare June 16, 2023 08:27

bkueng previously approved these changes Jun 16, 2023

View reviewed changes

dagar reviewed Jun 16, 2023

View reviewed changes

bkueng dismissed their stale review via 763cb10 June 19, 2023 09:17

bkueng force-pushed the pr-dataman_refactoring branch from 763cb10 to ea31874 Compare June 19, 2023 09:41

bkueng force-pushed the pr-dataman_refactoring branch from ea31874 to 52ad2e3 Compare June 19, 2023 13:43

dagar reviewed Jun 21, 2023

View reviewed changes

Igor-Misic and others added 21 commits July 24, 2023 10:29

mission: integrate dataman cache

2182d68

dataman_client: prevent loading duplicates in the cache

e4aef1f

geofence: integrate dataman cache

5f4136b

rtl: integrate dataman cache

e44596a

dataman_client: increase px4_poll timeout to 100_ms

f4b9a95

mavlink_mission: remove locking mechanism

d757b69

mavlink_mission & mission: move dataman update to mission

6bbd315

dataman: remove locking mechanism

f2569fc

dataman: add perf_counter

421af2e

GeofenceBreachAvoidanceTest: bug fix

eb94d83

dataman: update compat version

d4f42e5

A version update is needed since the dataman is showing errors if data doesn't exist or if it is wrongly stored. This will force default data to be initialized.

boards: remove camera capture and trigger from test builds

180c5c6

dataman: add guards for max item value

00baca0

ci: allow tests dataman to pass

90ed271

tests dataman will trigger errors and run_nsh_cmd.py is not written to detect only when a test fails. This is properly tested with run_tests.py

navigator: use mission topic to notify about geofence & safe point ch…

5d68917

…anges This avoids the need to regularly access dataman for checking.

navigator: defer saving mission until disarmed

1666498

Avoids unnecessary dataman accesses

boards/diatone/mamba-f405-mk2: disable modules to reduce flash

6a260e8

boards/px4/fmu-v5/test: disable modules to reduce flash

78db395

HIL: add --ignore-stdout-errors to run_nsh_cmd.py and use for 'tests …

efdda5b

…dataman' As this test is expected to contain errors.

fix dataman: treat 0 as valid return for read & ensure to init buffer

d852912

This can happen e.g. when the item is cleared.

mision: only run mission feasibility checks when mission updated

b5a528b

Instead of also when geofence/safe points updated. This prevents reporting multiple times.

bkueng force-pushed the pr-dataman_refactoring branch from 3730c7e to b5a528b Compare July 24, 2023 08:30

px4/fmu-v6x: disable batt_smbus for now to reduce flash

9e56674

tstastny approved these changes Jul 24, 2023

View reviewed changes

bkueng merged commit 8e8b35d into main Jul 24, 2023
87 checks passed

bkueng deleted the pr-dataman_refactoring branch July 24, 2023 11:10

KonradRudin mentioned this pull request Aug 7, 2023

Add mission interface #20601

Closed

Igor-Misic mentioned this pull request Aug 14, 2023

dataman file contains random data after creation #21278

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring dataman to remove locking mechanisms and to use uORB #21594

Refactoring dataman to remove locking mechanisms and to use uORB #21594

Igor-Misic commented May 15, 2023 •

edited

julianoes commented May 30, 2023

bkueng left a comment

dagar Jun 16, 2023

bkueng Jun 19, 2023

dagar commented Jun 16, 2023

bkueng commented Jun 19, 2023

tstastny commented Jun 19, 2023

tstastny commented Jun 21, 2023

dagar Jun 21, 2023

dagar commented Jun 21, 2023

bkueng commented Jul 24, 2023

DronecodeBot commented Sep 15, 2023

Refactoring dataman to remove locking mechanisms and to use uORB #21594

Refactoring dataman to remove locking mechanisms and to use uORB #21594

Conversation

Igor-Misic commented May 15, 2023 • edited

julianoes commented May 30, 2023

bkueng left a comment

Choose a reason for hiding this comment

dagar Jun 16, 2023

Choose a reason for hiding this comment

bkueng Jun 19, 2023

Choose a reason for hiding this comment

dagar commented Jun 16, 2023

bkueng commented Jun 19, 2023

tstastny commented Jun 19, 2023

tstastny commented Jun 21, 2023

dagar Jun 21, 2023

Choose a reason for hiding this comment

dagar commented Jun 21, 2023

bkueng commented Jul 24, 2023

DronecodeBot commented Sep 15, 2023

Igor-Misic commented May 15, 2023 •

edited