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

lora: Support RISC-V build, update to latest libtock-c and move to examples #432

Merged
merged 12 commits into from
Oct 25, 2024

Conversation

alistair23
Copy link
Contributor

No description provided.

@alistair23 alistair23 force-pushed the alistair/lora-example branch 10 times, most recently from 432e65e to 2fff49e Compare May 16, 2024 12:12
Copy link
Contributor

@bradjc bradjc left a comment

Choose a reason for hiding this comment

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

I don't think this should be upstreamed in this form. Adding RadioLib as a library like all other libtock-c external libraries would be great.

But this is too different from our current structure (adding maintenance overhead) and is too reliant on an external repository. This can lead to problems, like updating newlib to a version not here: https://github.com/jgromes/RadioLib/blob/master/examples/NonArduino/Tock/CMakeLists.txt and then libtock-c CI doesn't pass.

@alistair23
Copy link
Contributor Author

This feels a little like moving goal posts. This was moved to wip because it didn't build for RISC-V and would break the CI. Both of those issues are fixed. But now it has to include a custom build infrastructure.

The newlib path issue could easily be fixed by not using the version in the path, which is probably a good idea anyway.

I will have a look at re-implementing the build system, but I feel that this could be accepted in the meantime.

@alistair23 alistair23 force-pushed the alistair/lora-example branch from 2fff49e to 79a859b Compare June 17, 2024 09:25
@alistair23
Copy link
Contributor Author

I created #443 to help with the hardcoded version paths

I also updated this to use TockLibrary.mk which should address the current issue

@alistair23
Copy link
Contributor Author

alistair23 commented Jun 17, 2024

This builds locally for me, in the CI I see

 (insn 179 178 46 8 (set (reg:SI 3 r3 [168])
        (mem/u/c:SI (plus:SI (reg:SI 12 ip [169])
                (unspec:SI [
                        (symbol_ref/u:SI ("*.LC1") [flags 0x2])
                    ] UNSPEC_PIC_SYM)) [0  S4 A32])) "../../../examples/lora/RadioLib/src/modules/LR11x0/LR11x0.cpp":629:16 929 {*thumb1_movsi_insn}
     (expr_list:REG_EQUAL (symbol_ref/u:SI ("*.LC1") [flags 0x2])
        (nil)))
during RTL pass: postreload
../../../examples/lora/RadioLib/src/modules/LR11x0/LR11x0.cpp:641:1: internal compiler error: in extract_constrain_insn, at recog.c:2195
Please submit a full bug report,
with preprocessed source if appropriate.
See <https://gcc.gnu.org/bugs/> for instructions.
make: *** [../../..//TockLibrary.mk:175: ../../..//examples/lora/RadioLib/build/cortex-m0/RadioLib/examples/lora/RadioLib/src/modules/LR11x0/LR11x0.o] Error 1

So maybe a GCC bug in older versions that pops up when not using the cmake infrastructure? Thoughts @bradjc?

@alistair23 alistair23 force-pushed the alistair/lora-example branch 5 times, most recently from 2d5ab8a to 9f7c65b Compare June 18, 2024 01:31
@lschuermann
Copy link
Member

Okay, so this builds with libtock-c's Makefiles and with RadioLib's cmake? Is there a good reason for us to keep using cmake if the libtock-c Makefiles work?

@alistair23
Copy link
Contributor Author

The cmake is a useful fall back. It's part of the RadioLib CI and tested regularly. The Tock system hits a GCC bug so it's isn't as robust

Copy link
Contributor

@bradjc bradjc left a comment

Choose a reason for hiding this comment

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

It's fine to build with cmake out-of-tree, but we need PIC with our build system and want to avoid the overhead of supporting multiple build variations upstream.

Makefile Outdated Show resolved Hide resolved
Comment on lines -32 to -35
ifeq ($(strip $($(LIBNAME)_SRCS)),)
$(error Library "$(LIBNAME)" has no SRCS?)
endif

Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@alistair23 alistair23 Jun 18, 2024

Choose a reason for hiding this comment

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

Basically when running the formatter in the CI, the submodules aren't cloned. So no SRCs are found as there are no wildcards.

This avoids that error.

@alistair23 alistair23 force-pushed the alistair/lora-example branch from 9f7c65b to d5df181 Compare June 18, 2024 23:02
@alistair23
Copy link
Contributor Author

It's fine to build with cmake out-of-tree, but we need PIC with our build system and want to avoid the overhead of supporting multiple build variations upstream.

Yeah, the cmake uses PIC as well.

The Tock system fails to build the LR11x0 module in the CI due to some GCC bug. So it's worth keeping the cmake around as an option for people who want to use the LR11x0 module.

@ppannuto
Copy link
Member

ppannuto commented Jul 8, 2024

Hmm.. I'm of two minds on the cmake issue.

I do think there is probably some utility in an example that shows how one could integrate libtock with an external build system, but I'm also very wary of keeping anything like that around as a first-class example that we have limited control over and an obligation to support :/.

If we keep it around, I'd want CI to test it. I also have gone through Herculean efforts to ensure that minimal/standard tooling (i.e. just vanilla make) is all that is needed to get up-and-running with Tock, which we've gotten a ton of positive feedback on over the years as a design choice, and requiring cmake would run against that. I also try very hard to make sure that anything that runs as CI in the cloud is 'easy' to run locally; though there it's probably easy to follow a path similar to qemu where the local CI can run it, but wouldn't pull it in by default.... /endStreamOfConsciousness

I think I lean at the moment towards:

  • We should include the cmake as example
  • It should be built in a separate, non-required-to-merge "third-party-build" or similar CI job; this lets us see when something goes awry, while de-risking a third-party change holding up core development

@alistair23 alistair23 force-pushed the alistair/lora-example branch 2 times, most recently from 158fafb to e0ddbe6 Compare July 9, 2024 00:22
@alistair23
Copy link
Contributor Author

I removed the cmake option

@alistair23
Copy link
Contributor Author

Ready to go!

ppannuto
ppannuto previously approved these changes Oct 9, 2024
@ppannuto ppannuto requested a review from bradjc October 9, 2024 16:41
Copy link
Contributor

@bradjc bradjc left a comment

Choose a reason for hiding this comment

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

This doesn't build for me.

First because there is no Radiolib library cloned (need Makefile.setup)

Then:

❯ make V=1

**************************************************
TOCK USERLAND BUILD SYSTEM -- VERBOSE BUILD
**************************************************
Config:
GIT: 2.x-legacy-api-446-gb17e1a9a
arm-none-eabi-gcc --version: arm-none-eabi-gcc (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 13.3.1 20240614 Copyright (C) 2023 Free Software Foundation, Inc. This is free software; see the source for copying conditions.  There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
riscv64-unknown-elf-gcc --version: riscv64-unknown-elf-gcc (g2ee5e430018-dirty) 12.2.0 Copyright (C) 2022 Free Software Foundation, Inc. This is free software; see the source for copying conditions.  There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
LAYOUT=../../../userland_generic.ld
MAKEFLAGS= -r -R
PACKAGE_NAME=sensor-receive
TOCK_ARCHS=cortex-m0 cortex-m3 cortex-m4 cortex-m7 rv32imac rv32imc
TOCK_TARGETS=cortex-m0 cortex-m3 cortex-m4 cortex-m7 rv32imac|rv32imac.0x20040080.0x80002800|0x20040080|0x80002800 rv32imac|rv32imac.0x403B0080.0x3FCC0000|0x403B0080|0x3FCC0000 rv32imc|rv32imc.0x41000080.0x42008000|0x41000080|0x42008000 rv32imc|rv32imc.0x00080080.0x40008000|0x00080080|0x40008000 rv32imc|rv32imc.0x20030080.0x10005000|0x20030080|0x10005000 rv32imc|rv32imc.0x20030880.0x10008000|0x20030880|0x10008000 rv32imc|rv32imc.0x20032080.0x10008000|0x20032080|0x10008000 rv32imc|rv32imc.0x20034080.0x10008000|0x20034080|0x10008000 rv32imac|rv32imac.0x40430080.0x80004000|0x40430080|0x80004000 rv32imac|rv32imac.0x40440080.0x80007000|0x40440080|0x80007000 rv32imc|rv32imc.0x20300080.0x20602000|0x20300080|0x20602000
TOCK_USERLAND_BASE_DIR=../../..
TOOLCHAIN=
**************************************************

arm-none-eabi-gcc -std=gnu11 -Wbad-function-cast  -Wmissing-prototypes  -Wnested-externs  -Wold-style-definition  -Wjump-misses-init  -frecord-gcc-switches -gdwarf-2 -Os -fdata-sections -ffunction-sections -fstack-usage -D_FORTIFY_SOURCE=2 -Wall -Wextra -I../../.. -Wdate-time  -Wfloat-equal  -Wformat-nonliteral  -Wformat-security  -Wformat-y2k  -Winit-self  -Wmissing-declarations  -Wmissing-field-initializers  -Wmissing-format-attribute  -Wmissing-noreturn  -Wmultichar  -Wpointer-arith  -Wredundant-decls  -Wshadow  -Wunused-macros  -Wunused-parameter  -Wwrite-strings  -isystem ../../../RadioLib/RadioLib/src -isystem ../../../RadioLib/RadioLib/examples/NonArduino/Tock -Wstack-usage=2048 -Wlogical-op  -Wtrampolines  -Wl,--emit-relocs -fPIC -mthumb -mfloat-abi=soft -msingle-pic-base -mpic-register=r9 -mno-pic-data-is-text-relative -isystem ../../../lib/libtock-newlib-4.3.0.20230120/arm/arm-none-eabi/include -isystem ../../../lib/libtock-libc++-13.2.0/arm/arm-none-eabi/include/c++/13.2.0 -isystem ../../../lib/libtock-libc++-13.2.0/arm/arm-none-eabi/include/c++/13.2.0/arm-none-eabi -mcpu=cortex-m0 -march=armv6s-m -Wl,--warn-common -Wl,--gc-sections -Wl,--build-id=none  -Xlinker --defsym=STACK_SIZE=2048 -Xlinker --defsym=APP_HEAP_SIZE=1024 -Xlinker --defsym=KERNEL_HEAP_SIZE=1024 -T build/cortex-m0/cortex-m0.custom.ld -nostdlib -Wl,--start-group  build/cortex-m0/main.o    ../../../libtock/build/cortex-m0/libtock.a ../../../libtock-sync/build/cortex-m0/libtocksync.a ../../../RadioLib/build/cortex-m0/RadioLib.a ../../../lib/libtock-newlib-4.3.0.20230120/arm/arm-none-eabi/lib/thumb/v6-m/nofp/libc.a ../../../lib/libtock-newlib-4.3.0.20230120/arm/arm-none-eabi/lib/thumb/v6-m/nofp/libm.a ../../../lib/libtock-libc++-13.2.0/arm/arm-none-eabi/lib/thumb/v6-m/nofp/libstdc++.a ../../../lib/libtock-libc++-13.2.0/arm/arm-none-eabi/lib/thumb/v6-m/nofp/libsupc++.a ../../../lib/libtock-libc++-13.2.0/arm/lib/gcc/arm-none-eabi/13.2.0/thumb/v6-m/nofp/libgcc.a -Wl,--end-group -Wl,-Map=build/cortex-m0/cortex-m0.Map -o build/cortex-m0/cortex-m0.elf
/Applications/ArmGNUToolchain/13.3.rel1/arm-none-eabi/bin/../lib/gcc/arm-none-eabi/13.3.1/../../../../arm-none-eabi/bin/ld: build/cortex-m0/main.o: in function `TockHal::TockHal()':
/Users/bradjc/git/libtock-c/examples/lora/sensor-receive/../../../RadioLib/RadioLib/examples/NonArduino/Tock/libtockHal.h:87:(.text.startup.main+0x28): undefined reference to `RadioLibHal::RadioLibHal(unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long)'
/Applications/ArmGNUToolchain/13.3.rel1/arm-none-eabi/bin/../lib/gcc/arm-none-eabi/13.3.1/../../../../arm-none-eabi/bin/ld: build/cortex-m0/main.o: in function `main':
/Users/bradjc/git/libtock-c/examples/lora/sensor-receive/main.cc:32:(.text.startup.main+0x4a): undefined reference to `Module::Module(RadioLibHal*, unsigned long, unsigned long, unsigned long, unsigned long)'
/Applications/ArmGNUToolchain/13.3.rel1/arm-none-eabi/bin/../lib/gcc/arm-none-eabi/13.3.1/../../../../arm-none-eabi/bin/ld: /Users/bradjc/git/libtock-c/examples/lora/sensor-receive/main.cc:33:(.text.startup.main+0x5a): undefined reference to `SX1262::SX1262(Module*)'
/Applications/ArmGNUToolchain/13.3.rel1/arm-none-eabi/bin/../lib/gcc/arm-none-eabi/13.3.1/../../../../arm-none-eabi/bin/ld: /Users/bradjc/git/libtock-c/examples/lora/sensor-receive/main.cc:38:(.text.startup.main+0x86): undefined reference to `SX1262::begin(float, float, unsigned char, unsigned char, unsigned char, signed char, unsigned short, float, bool)'
/Applications/ArmGNUToolchain/13.3.rel1/arm-none-eabi/bin/../lib/gcc/arm-none-eabi/13.3.1/../../../../arm-none-eabi/bin/ld: build/cortex-m0/main.o:(.data.rel.ro._ZTI7TockHal[_ZTI7TockHal]+0x8): undefined reference to `typeinfo for RadioLibHal'
/Applications/ArmGNUToolchain/13.3.rel1/arm-none-eabi/bin/../lib/gcc/arm-none-eabi/13.3.1/../../../../arm-none-eabi/bin/ld: build/cortex-m0/main.o:(.data.rel.ro._ZTV7TockHal[_ZTV7TockHal]+0x4c): undefined reference to `RadioLibHal::tone(unsigned long, unsigned int, unsigned long)'
/Applications/ArmGNUToolchain/13.3.rel1/arm-none-eabi/bin/../lib/gcc/arm-none-eabi/13.3.1/../../../../arm-none-eabi/bin/ld: build/cortex-m0/main.o:(.data.rel.ro._ZTV7TockHal[_ZTV7TockHal]+0x50): undefined reference to `RadioLibHal::noTone(unsigned long)'
/Applications/ArmGNUToolchain/13.3.rel1/arm-none-eabi/bin/../lib/gcc/arm-none-eabi/13.3.1/../../../../arm-none-eabi/bin/ld: build/cortex-m0/main.o:(.data.rel.ro._ZTV7TockHal[_ZTV7TockHal]+0x58): undefined reference to `RadioLibHal::pinToInterrupt(unsigned long)'
collect2: error: ld returned 1 exit status
make: *** [build/cortex-m0/cortex-m0.elf] Error 1

Also I've seen

main.cc: In function 'int main()':
main.cc:53:38: warning: use of old-style cast to 'uint8_t*' {aka 'unsigned char*'} [-Wold-style-cast]
   53 |     state = radio->receive((uint8_t*)buffer, BUFFER_LEN);
      |                                      ^~~~~~
      |                            ----------------
      |                            reinterpret_cast<uint8_t*> (buffer)

RadioLib/Makefile.app Outdated Show resolved Hide resolved
@alistair23
Copy link
Contributor Author

Weird. It builds fine in the CI and locally for me.

I pushed some updates that might help you

@alistair23
Copy link
Contributor Author

Ping!

Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

I did a fresh checkout of this on my laptop and my desktop, both built for me — Brad, do the most recent changes fix your local build, o/w can look at testing with your exact compiler version.

It is... unfortunate that there's such a flood of warnings from the RadioLib codebase, and it does bury the one real [ultimately benign] cast warning in the 'local' tock code Brad flagged; we should fix that at least probably.

alistair23 and others added 12 commits October 23, 2024 10:23
avoid a wildcard with `rm -rf` in a script, and set the script
to error out if it's somehow not in the directory we expect
before `rm`'ing
Signed-off-by: Alistair Francis <[email protected]>
Signed-off-by: Alistair Francis <[email protected]>
Signed-off-by: Alistair Francis <[email protected]>
Signed-off-by: Alistair Francis <[email protected]>
Signed-off-by: Alistair Francis <[email protected]>
@alistair23 alistair23 force-pushed the alistair/lora-example branch from ccfd624 to b8ab66d Compare October 23, 2024 00:24
@alistair23
Copy link
Contributor Author

Fixed the reinterpret_cast warning in the Tock code

@hudson-ayers hudson-ayers added this pull request to the merge queue Oct 25, 2024
Merged via the queue into tock:master with commit 25a49c4 Oct 25, 2024
2 checks passed
@alistair23 alistair23 deleted the alistair/lora-example branch October 25, 2024 20:09
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.

5 participants