Added capability to specify the path to the mavlink xml files#2670
Added capability to specify the path to the mavlink xml files#2670JonasVautherin merged 2 commits intomavlink:mainfrom
Conversation
|
This one replaces the previous change for specifying the xml filepaths to the mavlink xml files. This change shows up as cmake optional path. I am sorry for mixing up with the other change, lack of experience with PRs here... |
third_party/tinyxml2/CMakeLists.txt
Outdated
| "-DCMAKE_POSITION_INDEPENDENT_CODE=ON" | ||
| "-DBUILD_SHARED_LIBS=OFF" | ||
| "-DBUILD_TESTS=OFF" | ||
| "-DBUILD_TESTING=OFF" |
There was a problem hiding this comment.
This should stay in your other PR about tinyxml2 😇
| set(MAVSDK_SOVERSION_STRING ${MAVSDK_VERSION_MAJOR}) | ||
| set(MAVSDK_VERSION_STRING ${MAVSDK_VERSION_MAJOR}.${MAVSDK_VERSION_MINOR}.${MAVSDK_VERSION_PATCH}) | ||
|
|
||
| set(MAVLINK_XML_PATH "" CACHE PATH "Where the mavlink xml files are located.") |
There was a problem hiding this comment.
Yeah that good to me, I don't think that people would want to set all 4 mavlink paths 👍
src/mavsdk/core/CMakeLists.txt
Outdated
| if (MAVLINK_XML_PATH) | ||
| set(MAVLINK_MINIMAL_XML "${MAVLINK_XML_PATH}/minimal.xml") | ||
| set(MAVLINK_STANDARD_XML "${MAVLINK_XML_PATH}/standard.xml") | ||
| set(MAVLINK_COMMON_XML "${MAVLINK_XML_PATH}/common.xml") | ||
| set(MAVLINK_ARDUPILOTMEGA_XML "${MAVLINK_XML_PATH}/ardupilotmega.xml") | ||
| else() | ||
| set(MAVLINK_MINIMAL_XML "${DEPS_INSTALL_PATH}/include/mavlink/message_definitions/v1.0/minimal.xml") | ||
| set(MAVLINK_STANDARD_XML "${DEPS_INSTALL_PATH}/include/mavlink/message_definitions/v1.0/standard.xml") | ||
| set(MAVLINK_COMMON_XML "${DEPS_INSTALL_PATH}/include/mavlink/message_definitions/v1.0/common.xml") | ||
| set(MAVLINK_ARDUPILOTMEGA_XML "${DEPS_INSTALL_PATH}/include/mavlink/message_definitions/v1.0/ardupilotmega.xml") | ||
| endif() |
There was a problem hiding this comment.
I'm nitpicking a bit, but wouldn't it be a bit easier if we did something like:
if(NOT MAVLINK_XML_PATH)
set(MAVLINK_XML_PATH "${DEPS_INSTALL_PATH}/include/mavlink/message_definitions/v1.0")
endif()
set(MAVLINK_MINIMAL_XML "${MAVLINK_XML_PATH}/minimal.xml")
set(MAVLINK_STANDARD_XML "${MAVLINK_XML_PATH}/standard.xml")
set(MAVLINK_COMMON_XML "${MAVLINK_XML_PATH}/common.xml")
set(MAVLINK_ARDUPILOTMEGA_XML "${MAVLINK_XML_PATH}/ardupilotmega.xml")What do you think?
There was a problem hiding this comment.
yes, good point indeed. I've gone ahead and made the PR. Hope that this is the right way to do it (still unfamiliar with github PRs)
There was a problem hiding this comment.
Hang on, we should not have different xmls, actually! That's because if you choose common.xml it will include minimal.xml and standard.xml.
And if you select ardupilotmega.xml, it includes common.xml. I guess we don't follow these includes and that's why we need to set it manually. But probably something to improve in the future.
There was a problem hiding this comment.
Indeed, xml files include 'each other'. I am confused now, does that have to do with my changes? I guess not, just double checking...
There was a problem hiding this comment.
@julianoes: is that blocking for this PR, or should we merge it?
There was a problem hiding this comment.
Assuming the PR doesn't change the behaviour, not a blocker.
There was a problem hiding this comment.
Right, that's the case, the PR does not change the behaviour.
There was a problem hiding this comment.
Yeah I think it's fine 👍
|
Ok, thanks for your comments. I have cleaned up the PR and I think it's now ready to go. Please review. |
|
Perfect, glad to be able to contribute something. Great work you are doing. |
6e19185 to
dedc6e2
Compare
|
No description provided.