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

Fix workflows #60

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

MrJake222
Copy link

Out of spite of it failing on my other PR

Upped SDK to 1.5.1 (1.5.0 has some implicit includes and doesn't compile with newest GCC (raspberrypi/pico-sdk#1367).

Problems:

  • pico_flash example name conflicts with pico sdk lib,
  • pico_ice_blinky has some weird errors.

They're disabled for now.

Main CMakeLists.txt compiles sdk/pico_sdk and
subfolders just link against it
Much faster than bash for loop (compiles sdks separately)
@MrJake222
Copy link
Author

MrJake222 commented Nov 6, 2024

Discovered a problem:

[ 35%] Building C object pico_fpga_io/CMakeFiles/pico_fpga_io.dir/main.c.obj
/home/runner/work/pico-ice-sdk/pico-ice-sdk/examples/pico_fpga_io/main.c: In function 'main':
/home/runner/work/pico-ice-sdk/pico-ice-sdk/examples/pico_fpga_io/main.c:46:9: warning: implicit declaration of function 'ice_fpga_write'; did you mean 'ice_fpga_init'? [-Wimplicit-function-declaration]
   46 |         ice_fpga_write(0x55551001, buffer, sizeof buffer);
      |         ^~~~~~~~~~~~~~
      |         ice_fpga_init
/home/runner/work/pico-ice-sdk/pico-ice-sdk/examples/pico_fpga_io/main.c:47:9: warning: implicit declaration of function 'ice_fpga_read'; did you mean 'ice_flash_read'? [-Wimplicit-function-declaration]
   47 |         ice_fpga_read(0x55551001, buffer, sizeof buffer);
      |         ^~~~~~~~~~~~~
      |         ice_flash_read

ice_fpga.h doesn't export all functions.

Unusual changes:
Moved #ifdef ...CDC... up over void tud_cdc_line_coding_cb() in src/ice_usb.c. Was failing with unknown type cdc_line_coding_t.

Copy link
Collaborator

@josuah josuah left a comment

Choose a reason for hiding this comment

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

Thank you for this maintenance effort! Some things clearly needs fixing. Others is "worse design on purpose" (replicating the same files in every example) for improving the "get started" workflow.

Comment on lines +319 to +320
#if ICE_USB_UART0_CDC || ICE_USB_UART1_CDC || ICE_USB_FPGA_CDC || ICE_USB_SPI_CDC

Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes sense, very good point!

git -C pico-sdk submodule update --init
git -C lib/pico-sdk submodule update --init
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, only this? That's a lot fewer than I feared. Thanks for the fix!

Copy link
Collaborator

Choose a reason for hiding this comment

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

The idea of replicating the whole project declaration in every example is that it is possible to copy-paste an example directory to start a new project, replace the symlinks by git submodules, and that's it.

Otherwise, this means a lot of CMakeList.txt changes need to be done, and there is nowhere where it is documented anymore. Given every project needs a slightly different CMake setup, this was changed from a main CMakeLists.txt for all examples to each example having its own CMakeLists.txt.

I agree it is easier to maintain as you propose, though.

Comment on lines +16 to +17
# colides with pico sdk target
#add_subdirectory(pico_flash)
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is right, this needs fixing. Maybe an example_pico_... and example_ice_... prefix is the way.
Thank you for raising this.

Comment on lines +22 to +23
# doesn't build
#add_subdirectory(pico_ice_blinky)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very good to know! This will be addressed in next release.

Comment on lines +6 to +14
#include "ice_cram.h"
#include "ice_flash.h"
#include "ice_fpga.h"
#include "ice_comm.h"
#include "ice_led.h"
#include "ice_pmod.h"
#include "ice_spi.h"
#include "ice_sram.h"
#include "ice_usb.h"
#include "ice_wishbone.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point, this was not kept in-sync.

@MrJake222
Copy link
Author

The idea of replicating the whole project declaration in every example is that it is possible to copy-paste an example directory to start a new project, replace the symlinks by git submodules, and that's it.

I guess you should decide then of you want a easy-to-go example set or a testing infrastructure. Another approach I thought about (because building every single one of them as a separate project with pico-sdk and ice-sdk is too long of a process) is to create a one big cmake file which will build all the examples for workflows. You'd need to include files manually there and duplicate a lot of what CMakeFiles already specify, but it's another solution.

@josuah
Copy link
Collaborator

josuah commented Nov 12, 2024

Each example is tested on Linux and Windows (I do not have a Mac OSX but I would also tests there if I had) at every release, so CI is not there as a complete line of defense against bugs as it requires a USB host and actual hardware to test.

Some projects have a "build all" test that simply grabs all the sources, and build them. That would be some good enough approach for CI.

Given the default firmware makes use of (almost) all the APIs, that could be some good candidate.
Good point about minimizing the build time. Github CI is not a magic pool of endless resources.

@MrJake222
Copy link
Author

Some projects have a "build all" test that simply grabs all the sources, and build them. That would be some good enough approach for CI.

So one big CMakeLists.txt sounds good enough?

@josuah
Copy link
Collaborator

josuah commented Nov 12, 2024

Some things on that repo named "examples" are actually tests (like the C++ example, just there for CI). There could be a tests directory with a single CMakeLists.txt.

Currently, turning an example into a project only requires replacing the symlinks by git submodules, if we can keep that this is great! https://pico-ice.tinyvision.ai/md_getting_started.html#autotoc_md8

Does it make sense? Maybe I missed something?

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