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

socktap: Adding the Autotalks link layer #151

Merged
merged 14 commits into from
Aug 20, 2023
Merged

Conversation

khevessy
Copy link
Contributor

@khevessy khevessy commented Apr 13, 2022

This adds Autotalks link layer to the socktap example, working on both Craton and Secton devices. Links with Autotalks libraries. Also adds possibility to build for Craton (cross-compilation for ARM using the toolchain supplied with Autotalks SDK).

Initialization of the device can be taken from the Autotalks example, as I propose in the documentation page (I used almost exactly their code, therefore I think it cannot be in the library).

I should add that my knowledge of CMake is not great, so that part may require some further work.

Copy link
Owner

@riebl riebl left a comment

Choose a reason for hiding this comment

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

Thanks for your effort! Unfortunately, I cannot test your code on an actual Autotalks device as I have no longer access to any.
I have added several comments where I would like to see changes before merging it into the master branch. Please don't be shocked by these comments, I really appreciate your contribution!

tools/socktap/link_layer.cpp Outdated Show resolved Hide resolved
tools/socktap/main.cpp Outdated Show resolved Hide resolved
tools/socktap/autotalks.hpp Outdated Show resolved Hide resolved
tools/socktap/autotalks.cpp Show resolved Hide resolved
tools/socktap/autotalks.hpp Show resolved Hide resolved
cmake/FindAutotalks.cmake Outdated Show resolved Hide resolved
cmake/FindAutotalks.cmake Outdated Show resolved Hide resolved
find_path(
AUTOTALKS_ROOT
NAMES "include/atlk/v2x_service.h"
PATHS $ENV{HOME}
Copy link
Owner

Choose a reason for hiding this comment

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

So you expect the API to be located at /home/your_user/autotalks_craton_api? Is this suggested by Autotalks?

Copy link
Contributor Author

@khevessy khevessy Apr 19, 2022

Choose a reason for hiding this comment

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

I expect the user to create symbolic link from /home/your_user/autotalks_craton_api or /home/your_user/autotalks_secton_api to SDK root, I thought that it was simpler as there does not seem to be any consistent path defined by Autotalks. However, the SDK itself is found in secton-sdk-5.14.0-rel_sec_USB or craton2-sdk-5.14.0-rel_cr2_lnx for version 5.14.0, so maybe that could be used somehow?

Copy link
Owner

Choose a reason for hiding this comment

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

Would you please add a note about this expectation to autotalks-sdk-build.md?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was already there but I tried to make the explanation more clear

tools/socktap/CMakeLists.txt Outdated Show resolved Hide resolved
tools/socktap/autotalks_link.cpp Outdated Show resolved Hide resolved
- moved v2x_device_init() to AutotalksLink constructor
- moved autotalks_device_deinit() to AutotalksLink destructor, main.cpp could be revert
- renamed v2x_device_init() to autotalks_device_init()
- added #error to to autotalks initialization to prevent silent build of non-functioning application
- wrapped functions from autotalks.hpp to vanetza::autotalks namespace
- renamed Toolchain-Autotalks.cmake to Toolchain-Autotalks-Craton.cmake
- added CMake variable for poky toolchain path
- removed debugging loop neglected in FindAutotalks.cmake
- renamed AUTOTALKS_EXECUTABLES to AUTOTALKS_OBJECTS in cmake
- moved Autotalks::AtlkRemote and Autotalks::AtlkLocal dependencies to FindAutotalks.cmake
- made constant for send buffer size bigger to be larger than MTU
@khevessy
Copy link
Contributor Author

khevessy commented Apr 19, 2022

I am not sure if you are happy with user manually creating the symbolic link to SDK root as mentioned above, but to me it seems good enough (as the path is not defined exactly by Autotalks).
Apart from that I hopefully solved all your requests.

@khevessy khevessy requested a review from riebl April 26, 2022 17:46
Copy link
Owner

@riebl riebl left a comment

Choose a reason for hiding this comment

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

I think I will merge this shortly :-)

)

set(AUTOTALKS_INCLUDE_DIRS "${AUTOTALKS_ROOT}/include" "${AUTOTALKS_ROOT}/ref_src/include" "${AUTOTALKS_ROOT}/depend/device/include" "${AUTOTALKS_ROOT}/src/include" "${AUTOTALKS_ROOT}/src/core/include")
set(AUTOTALKS_LIBS_DIR ${AUTOTALKS_ROOT}/output.sec/x86/lib)
Copy link
Owner

Choose a reason for hiding this comment

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

Are there only x86 libraries? May we want to check CMAKE_SYSTEM_PROCESSOR to select the right directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By x86, Autotalks code generation means that the system's native compiler is used (e.g. no cross-compilation) so in this case this will really be x86 on every platform (last year I tried SDK compilation on Raspberry PI and libraries were still in x86 folder). Autotalks documentation mentions that when Secton is controlled from Craton and we are cross-compiling for Craton, output folder is armv7-32. However, I did not try this possibility.


set(AUTOTALKS_INCLUDE_DIRS "${AUTOTALKS_ROOT}/include" "${AUTOTALKS_ROOT}/ref_src/include" "${AUTOTALKS_ROOT}/depend/device/include" "${AUTOTALKS_ROOT}/src/include" "${AUTOTALKS_ROOT}/src/core/include")

set(AUTOTALKS_LIBS_DIR ${AUTOTALKS_ROOT}/output.cr2_lnx/armv7-32/lib)
Copy link
Owner

Choose a reason for hiding this comment

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

Here it is always arm7-32 because that is simply the Craton processor's architecture, isn't it?
Edit: see also Toolchain-Autotalks-Craton.cmake which pins the architecture to ARM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think this will always stay the same as you say.

cmake/FindAutotalks.cmake Outdated Show resolved Hide resolved
int autotalks_device_init(void)
{
atlk_rc_t rc;
const char* arg[] = {"./test", "enx0002ccf00006"};
Copy link
Owner

Choose a reason for hiding this comment

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

black magic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have put the "enx0002ccf00006" network name in define and added a note to the documentation. I think it is hard-coded somewhere from Autotalks

find_path(
AUTOTALKS_ROOT
NAMES "include/atlk/v2x_service.h"
PATHS $ENV{HOME}
Copy link
Owner

Choose a reason for hiding this comment

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

Would you please add a note about this expectation to autotalks-sdk-build.md?

@khevessy
Copy link
Contributor Author

Note that socktap now does not work as stated here. After recent security changes, it cannot be compiled in the base repository either.

@riebl
Copy link
Owner

riebl commented Aug 20, 2023

Sorry, I did not notice that socktap cannot be compiled. I should add socktap to the CI!
Are you also suffering from the assertion failure mentioned by @lo-hei?

@riebl riebl merged commit 0dd221f into riebl:master Aug 20, 2023
5 checks passed
@riebl
Copy link
Owner

riebl commented Aug 20, 2023

Thanks for your hard work and patience @khevessy!

@khevessy
Copy link
Contributor Author

After your fix, I've tried socktap on Secton and Craton and it is working well.

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.

2 participants