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

Close replay log file after replay is finished, then exit #11264

Merged
merged 18 commits into from
Aug 28, 2019

Conversation

roangel
Copy link
Contributor

@roangel roangel commented Jan 22, 2019

Describe problem solved by the proposed pull request
When finishing log replay, close the log file and exit process. The main problem is that right now we do not have a method implemented for px4_request_shutdown() when we are in SITL, as discussed briefly here: http://discuss.px4.io/t/bug-calibration-gazebo-sitl/9138/18

@roangel roangel changed the title [WIP] Close replay log file after replay is finished, then exit Close replay log file after replay is finished, then exit Jan 23, 2019
@dagar dagar requested a review from bkueng February 5, 2019 15:16
Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @roangel, and sorry, I didn't see this earlier.

@@ -191,7 +191,11 @@ void shutdown_worker(void *arg)

} else {
PX4_WARN("Shutdown NOW. Good Bye.");
#if defined __PX4_POSIX
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather implement board_shutdown for SITL, then we don't need this ifdef.
px4_systemreset is incorrect here, as it's supposed to do a reboot.

@@ -205,7 +209,9 @@ void shutdown_worker(void *arg)
int px4_shutdown_request(bool reboot, bool to_bootloader)
{
// fail immediately if the board does not support the requested method
#ifndef __PX4_POSIX
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ifdef should not be required if board_shutdown is implemented for SITL

@stale
Copy link

stale bot commented Jul 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale
Copy link

stale bot commented Jul 24, 2019

Closing as stale.

@stale stale bot closed this Jul 24, 2019
@julianoes julianoes reopened this Jul 25, 2019
@stale stale bot removed the Admin: Wont fix label Jul 25, 2019
@julianoes
Copy link
Contributor

@roangel could you check the review comments so we can get this in?

@roangel
Copy link
Contributor Author

roangel commented Aug 19, 2019

I tried to do the implementation of board_shutdown method in its respective place (or at least what I think it is the place), but it is not working. Anyone has some input? Where should I put the implementation of the sitl shutdown method?

@bkueng
Copy link
Member

bkueng commented Aug 19, 2019

It looks right, does board_shutdown get called? What's the output you get?

@roangel
Copy link
Contributor Author

roangel commented Aug 19, 2019

Right now the compiler throws an error:
image

And when I solve it by adding the needed defines to boards/px4/sitl/src/board_config.h, I stumble upon this:
image

Any ideas?

@bkueng
Copy link
Member

bkueng commented Aug 20, 2019

This works:

diff --git a/boards/px4/sitl/src/sitl_board_shutdown.c b/boards/px4/sitl/src/sitl_board_shutdown.c
index 11a9850ff1..9ca9326a53 100644
--- a/boards/px4/sitl/src/sitl_board_shutdown.c
+++ b/boards/px4/sitl/src/sitl_board_shutdown.c
@@ -38,14 +38,15 @@
  */
 
 #include <px4_tasks.h>
-#include <drivers/boards/common/board_common.h>
+#include <board_config.h>
 
-__BEGIN_DECLS
-extern int board_shutdown(void) noreturn_function;
-static inline int board_register_power_state_notification_cb(power_button_state_notification_t cb) { return 0; }
-__END_DECLS
+int board_register_power_state_notification_cb(power_button_state_notification_t cb)
+{
+	return 0;
+}
 
-__EXPORT int board_shutdown()
+int board_shutdown()
 {
 	px4_systemreset(false);
+	return 0;
 }

@bkueng
Copy link
Member

bkueng commented Aug 21, 2019

We're getting closer. The build fails on MacOS and Windows for the CollisionPreventionTest build because of a missing dependency.

This might solve it:

diff --git a/boards/px4/sitl/src/CMakeLists.txt b/boards/px4/sitl/src/CMakeLists.txt
index fa201136ae..063e4f71f3 100644
--- a/boards/px4/sitl/src/CMakeLists.txt
+++ b/boards/px4/sitl/src/CMakeLists.txt
@@ -36,3 +36,6 @@ add_library(drivers_board
 	sitl_led.c
 	sitl_board_shutdown.c
 )
+
+add_dependencies(drivers_board prebuild_targets)
+
diff --git a/src/modules/simulator/ledsim/CMakeLists.txt b/src/modules/simulator/ledsim/CMakeLists.txt
index c8f9cbc012..5358958c98 100644
--- a/src/modules/simulator/ledsim/CMakeLists.txt
+++ b/src/modules/simulator/ledsim/CMakeLists.txt
@@ -32,4 +32,5 @@
 ############################################################################
 
 px4_add_library(drivers__ledsim led.cpp)
-target_link_libraries(drivers__ledsim PRIVATE drivers__led drivers_board)
+target_link_libraries(drivers__ledsim PRIVATE drivers__led)
+

@bkueng
Copy link
Member

bkueng commented Aug 27, 2019

I think I have it now:

diff --git a/boards/px4/sitl/src/CMakeLists.txt b/boards/px4/sitl/src/CMakeLists.txt
index 0c925b02c7..d3156ed0dc 100644
--- a/boards/px4/sitl/src/CMakeLists.txt
+++ b/boards/px4/sitl/src/CMakeLists.txt
@@ -37,4 +37,3 @@ add_library(drivers_board
 	sitl_board_shutdown.c
 )
 
-add_dependencies(drivers_board prebuild_targets)
diff --git a/platforms/posix/src/px4_layer/CMakeLists.txt b/platforms/posix/src/px4_layer/CMakeLists.txt
index ffa0638461..a7024ce972 100644
--- a/platforms/posix/src/px4_layer/CMakeLists.txt
+++ b/platforms/posix/src/px4_layer/CMakeLists.txt
@@ -56,7 +56,7 @@ add_library(px4_layer
 target_compile_definitions(px4_layer PRIVATE MODULE_NAME="px4")
 target_compile_options(px4_layer PRIVATE -Wno-cast-align) # TODO: fix and enable
 target_link_libraries(px4_layer PRIVATE work_queue px4_work_queue)
-target_link_libraries(px4_layer PRIVATE px4_daemon)
+target_link_libraries(px4_layer PRIVATE px4_daemon drivers_board)
 
 if(ENABLE_LOCKSTEP_SCHEDULER)
 	target_link_libraries(px4_layer PRIVATE lockstep_scheduler)

With this make tests works for me.

Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @roangel

@bkueng bkueng merged commit e50dd7c into PX4:master Aug 28, 2019
bozkurthan pushed a commit to bozkurthan/Firmware that referenced this pull request Sep 4, 2019
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.

3 participants