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

Tweak tests and consolidate pins target validation #21254

Merged

Conversation

thinkyhead
Copy link
Member

@thinkyhead thinkyhead commented Mar 5, 2021

  • Rename tests without -tests suffix.
  • Move run_tests to buildroot/bin.
  • Update mftest and run_tests with the new paths.
  • Replace target test with #include env_validate.h in most pins files.

@p3p
Copy link
Member

p3p commented Mar 5, 2021

I disagree completely with this, there are board specific options that require changing the platform build flags, making the env selection more confusing is unnecessary, BOARD_CUSTOM_BUILD_FLAGS works well for these edge cases.

@thinkyhead thinkyhead force-pushed the bf2_board_flags_PR branch from 5e83d83 to 20e6d68 Compare March 5, 2021 02:00
@thinkyhead
Copy link
Member Author

TBF the number of environments now existing already makes the use of the Auto Build Marlin extension practically necessary, and truly there is little guidance from board makers that "LPC1769" would be the right thing to select in PlatformIO for their board named after an animal.

@p3p
Copy link
Member

p3p commented Mar 5, 2021

Perhaps but this PR removes a useful feature, adds pointless tests, and will create confusion for people following current guides for the boards.

@thinkyhead
Copy link
Member Author

Someone building for RAMPS in Arduino IDE (or other AVR-oriented build environment) might assume that the BOARD_CUSTOM_BUILD_FLAGS in the pins file will be picked up by that IDE, but it won't.

While the concept might have some utility later, right now it applies only to a single case. The aim of the dependencies scripts was initially to prevent building CPP files that are not used, so PIO builds can be much faster (again) than Arduino IDE builds. It was not itself a dependency required to build Marlin correctly, whereas some other additions have gone that way. I'm disinclined to keep things that only cement more dependency on PIO.

@rhapsodyv
Copy link
Member

For context: #19468

BOARD_CUSTOM_BUILD_FLAGS was the reason for us to let our build script include pins file.

But, now with the lasts additions to the build script, we can achieve the same effect translating it to a feature.

For example:

MY_SPECIFIC_BOARD_FEATURE = build_flags=-DMY_BOARD_FOO

then, on pins file, just:

#define MY_SPECIFIC_BOARD_FEATURE

Anyway, none of our features that need extra libs or use scripts or use build_flags will work on arduino ide... so it’s not a issue only with BOARD_CUSTOM_BUILD_FLAGS...

About envs: I prefer having a env for at least a family of boards or chip. One env per board is a nightmare. Worst when we have two or three envs for one board. But too generic envs will mess with the editor intelisence... so we would always need to find a balance between the two.

@thinkyhead
Copy link
Member Author

Sure enough. I have all of that in mind as well.

@rhapsodyv
Copy link
Member

rhapsodyv commented Mar 5, 2021

About PIO dependency. PIO is basically a package/lib manager on top of SCONS.

If we let out the ability of automatic lib and tooling installation, everything else is plain SCONS.

Recently I migrate a project from IAR to SCONS (yes, just a Sconstruct SCONS script - similar to a makefile, but in python).
Then, moving the project to pio I just needed to create a platformio.ini.

Everything we does with "env" in python is not PlatformIO. It's just SCONS.

The most used building tools are plain makefile (old or simple projects), auto conf (linux masters only), CMake and SCONS.

What I mean is: support scons as a main and unique build tools is not necessarily bad, as most of the projects out there support only one build tool anyway. And our build tool is really powerful.

Maybe we can even integrate scons on arduino ide, I never searched about it.

@thinkyhead thinkyhead force-pushed the bf2_board_flags_PR branch from 20e6d68 to b19abb1 Compare March 5, 2021 02:26
@p3p
Copy link
Member

p3p commented Mar 5, 2021

This feature is currently allowing for skr v1.4 boards to work correctly without users knowing there is a platform config difference, unless that can be preserved I don't think it should be removed yet, adding board specific envs is always a bad idea.

@thinkyhead
Copy link
Member Author

Bringing SCons to other build environments seems like it would be a good idea. Of course, Arduino IDE is packaged to be a standalone application, so it would need to be rebuilt and contain its own Python interpreter.

@thinkyhead
Copy link
Member Author

It is also important to note that our stuff doesn't just depend on SCons, but also on being able to locate a working GCC compiler somewhere in the $PATH to do the initial pre-compile step to pull defines from H and CPP files into a Python dictionary.

@rhapsodyv
Copy link
Member

For this specific case of skr, I think it should be translated into a feature.

No one will build lpc on arduino ide.

And making it a feature, will not give a lot of support from users building in the wrong env....

@rhapsodyv
Copy link
Member

It is also important to note that our stuff doesn't just depend on SCons, but also on being able to locate a working GCC compiler somewhere in the $PATH to do the initial pre-compile step to pull defines from H and CPP files into a Python dictionary.

Yes, we use some PIO concepts and helpers. But just because we can. It give us some free sugar.

If we try to make it work on arduino ide, we probably need some ifs, but not that much. And for arduino, the tooling is only one and only one folder, isn't?

I will take a look if we can create some sort of arduino ide plugin.

@thinkyhead
Copy link
Member Author

adding board-specific envs is always a bad idea

Users are already forced to dig and learn that their board is "LPC1768" or "STM32F1_blah_blah" at some point, so I don't think it adds any special burden to make them do it once again.

But, if we really do want to make sure users select the right environment, we could have each environment set a -D flag to identify itself, and then each pins file can check that the correct env is selected (only when the identifier is actually set).

@p3p
Copy link
Member

p3p commented Mar 5, 2021

With the features .. feature of the platformio.ini we can maintain working SKR v1.4 builds without adding an env with something like the following, LPC will never build with Arduino IDE so I think its acceptable to require platformio for the build process on that platform.

pins_BTT_SKR_V1_4.h

-#ifndef BOARD_CUSTOM_BUILD_FLAGS
-  #define BOARD_CUSTOM_BUILD_FLAGS -DLPC_PINCFG_UART3_P4_28
-#endif
+#define IS_BTT_SKR_V1_4

platformio.ini [common_LPC] section

 custom_marlin.USES_LIQUIDCRYSTAL = arduino-libraries/LiquidCrystal@~1.0.7
 custom_marlin.NEOPIXEL_LED = Adafruit NeoPixel=https://github.com/p3p/Adafruit_NeoPixel/archive/1.5.0.zip
+custom_marlin.IS_BTT_SKR_V1_4 = build_flags=-DLPC_PINCFG_UART3_P4_28

@thinkyhead thinkyhead force-pushed the bf2_board_flags_PR branch 2 times, most recently from 20ba227 to 563a649 Compare March 5, 2021 04:12
@thisiskeithb
Copy link
Member

TBF the number of environments now existing already makes the use of the Auto Build Marlin extension practically necessary

At least with #21068, there's some guidance as soon as the user tries to build with the incorrect environment now.

@thinkyhead thinkyhead force-pushed the bf2_board_flags_PR branch 7 times, most recently from 7eaac6b to 04c46ad Compare March 5, 2021 05:32
@thinkyhead thinkyhead force-pushed the bf2_board_flags_PR branch 3 times, most recently from 1b6d8a0 to 795b181 Compare March 5, 2021 09:49
@thinkyhead thinkyhead force-pushed the bf2_board_flags_PR branch from 795b181 to 429ff54 Compare March 5, 2021 09:57
@thinkyhead thinkyhead force-pushed the bf2_board_flags_PR branch from 429ff54 to 2845ebd Compare March 5, 2021 10:17
@thinkyhead thinkyhead changed the title Drop BOARD_CUSTOM_BUILD_FLAGS Tweak tests and consolidate pins target validation Mar 5, 2021
@thinkyhead
Copy link
Member Author

With due deference to SCons as a newer/better build system than make/cmake, I've dropped the original aim of this PR and just done a bunch of reorganization instead.

@thinkyhead thinkyhead merged commit 3ea56ba into MarlinFirmware:bugfix-2.0.x Mar 5, 2021
@thinkyhead thinkyhead deleted the bf2_board_flags_PR branch March 5, 2021 10:30
@thisiskeithb
Copy link
Member

This PR brings back the "Can not remove temporary directory /github/Marlin/.pio/build. Please remove it manually to avoid build issues" build error.

Vertabreak added a commit to Vertabreak/Marlin that referenced this pull request Mar 5, 2021
Tweak tests, consolidate pins target validation (MarlinFirmware#21254)
@rhapsodyv
Copy link
Member

This PR brings back the "Can not remove temporary directory /github/Marlin/.pio/build. Please remove it manually to avoid build issues" build error.

Missing "env:" here:

platform = ${LPC1768.platform}

rmpel added a commit to rmpel/Marlin that referenced this pull request Mar 5, 2021
* bugfix-2.0.x: (135 commits)
  Tweak tests, consolidate pins target validation (MarlinFirmware#21254)
  [cron] Bump distribution date (2021-03-05)
  Fix multi-serial CRC error crash (MarlinFirmware#21249)
  Followup to MP_SCARA/TPARA patches (MarlinFirmware#21248)
  Remove extra G42
  Correct fan pins for MKS Robin Nano v3 (MarlinFirmware#21238)
  SMUFF => SMuFF (MarlinFirmware#21243)
  Implement G42, after all
  MK2_MULTIPLEXER dependency
  Update some py scripts
  Parking Extruder solenoid fix/cleanup
  Fix teensy35 tests
  [cron] Bump distribution date (2021-03-04)
  TPARA followup
  TPARA - 3DOF robot arm IK (MarlinFirmware#21005)
  misc. cleanup
  Improve opt_set (etc.) used for tests
  Fix MKS H43 compile (MarlinFirmware#21240)
  [cron] Bump distribution date (2021-03-03)
  Trust XY after Quiet Probing short sleep (MarlinFirmware#21237)
  ...

# Conflicts:
#	Marlin/Configuration.h
#	Marlin/Configuration_adv.h
thinkyhead added a commit to thisiskeithb/Marlin that referenced this pull request Mar 6, 2021
thinkyhead pushed a commit that referenced this pull request Mar 6, 2021
Missing commit from #21254
Co-authored-by: Scott Lahteine <github@thinkyhead.com>
rmpel added a commit to rmpel/Marlin that referenced this pull request Mar 6, 2021
* bugfix-2.0.x: (135 commits)
  Tweak tests, consolidate pins target validation (MarlinFirmware#21254)
  [cron] Bump distribution date (2021-03-05)
  Fix multi-serial CRC error crash (MarlinFirmware#21249)
  Followup to MP_SCARA/TPARA patches (MarlinFirmware#21248)
  Remove extra G42
  Correct fan pins for MKS Robin Nano v3 (MarlinFirmware#21238)
  SMUFF => SMuFF (MarlinFirmware#21243)
  Implement G42, after all
  MK2_MULTIPLEXER dependency
  Update some py scripts
  Parking Extruder solenoid fix/cleanup
  Fix teensy35 tests
  [cron] Bump distribution date (2021-03-04)
  TPARA followup
  TPARA - 3DOF robot arm IK (MarlinFirmware#21005)
  misc. cleanup
  Improve opt_set (etc.) used for tests
  Fix MKS H43 compile (MarlinFirmware#21240)
  [cron] Bump distribution date (2021-03-03)
  Trust XY after Quiet Probing short sleep (MarlinFirmware#21237)
  ...

# Conflicts:
#	Marlin/Configuration.h
#	Marlin/Configuration_adv.h
vyacheslav-shubin pushed a commit to vyacheslav-shubin/Marlin that referenced this pull request Mar 10, 2021
vyacheslav-shubin pushed a commit to vyacheslav-shubin/Marlin that referenced this pull request Mar 10, 2021
Missing commit from MarlinFirmware#21254
Co-authored-by: Scott Lahteine <github@thinkyhead.com>
vyacheslav-shubin pushed a commit to vyacheslav-shubin/Marlin that referenced this pull request Mar 10, 2021
vyacheslav-shubin pushed a commit to vyacheslav-shubin/Marlin that referenced this pull request Mar 10, 2021
Missing commit from MarlinFirmware#21254
Co-authored-by: Scott Lahteine <github@thinkyhead.com>
W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021
W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021
Missing commit from MarlinFirmware#21254
Co-authored-by: Scott Lahteine <github@thinkyhead.com>
thinkyhead pushed a commit that referenced this pull request Apr 30, 2021
Missing commit from #21254
Co-authored-by: Scott Lahteine <github@thinkyhead.com>
rmpel added a commit to rmpel/Marlin that referenced this pull request Feb 3, 2022
* bugfix-2.0.x: (135 commits)
  Tweak tests, consolidate pins target validation (MarlinFirmware#21254)
  [cron] Bump distribution date (2021-03-05)
  Fix multi-serial CRC error crash (MarlinFirmware#21249)
  Followup to MP_SCARA/TPARA patches (MarlinFirmware#21248)
  Remove extra G42
  Correct fan pins for MKS Robin Nano v3 (MarlinFirmware#21238)
  SMUFF => SMuFF (MarlinFirmware#21243)
  Implement G42, after all
  MK2_MULTIPLEXER dependency
  Update some py scripts
  Parking Extruder solenoid fix/cleanup
  Fix teensy35 tests
  [cron] Bump distribution date (2021-03-04)
  TPARA followup
  TPARA - 3DOF robot arm IK (MarlinFirmware#21005)
  misc. cleanup
  Improve opt_set (etc.) used for tests
  Fix MKS H43 compile (MarlinFirmware#21240)
  [cron] Bump distribution date (2021-03-03)
  Trust XY after Quiet Probing short sleep (MarlinFirmware#21237)
  ...

# Conflicts:
#	Marlin/Configuration.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants