analogdevicesinc / msdk

Software Development Kit for Analog Device's MAX-series microcontrollers
Apache License 2.0
60 stars 75 forks source link

Makefile does not recompile despite dependent header file changes on MSYS2 #528

Open jlj-ee opened 1 year ago

jlj-ee commented 1 year ago

I am not seeing recompilation happen if I edit a header file and then recompile. Minimal example to repro using VS Code hello_world example.

  1. Clean and build the hello_world project. Observe the build task terminal output:
    Loaded project.mk
    CC    main.c
    CC    C:/MaximSDK/Libraries/Boards/MAX32670/EvKit_V1/Source/board.c
    CC    C:/MaximSDK/Libraries/Boards/MAX32670/EvKit_V1/../Source/stdio.c
    CC    C:/MaximSDK/Libraries/Boards/MAX32670/EvKit_V1/../Source/led.c
    CC    C:/MaximSDK/Libraries/Boards/MAX32670/EvKit_V1/../Source/pb.c
    AS    C:/MaximSDK/Libraries/CMSIS/Device/Maxim/MAX32670/Source/GCC/startup_max32670.S
    CC    C:/MaximSDK/Libraries/Boards/MAX32670/EvKit_V1/Source/rom_stub.c
    CC    C:/MaximSDK/Libraries/CMSIS/Device/Maxim/MAX32670/Source/heap.c
    CC    C:/MaximSDK/Libraries/CMSIS/Device/Maxim/MAX32670/Source/system_max32670.c
    LD    <mydir>/examples/max32670_hello_world/build/max32670_hello_world.elf 
    arm-none-eabi-size --format=berkeley <mydir>/examples/max32670_hello_world/build/max32670_hello_world.elf
    text    data     bss     dec     hex filename
    35676    2500     628   38804    9794 <mydir>/examples/max32670_hello_world/build/max32670_hello_world.elf
  2. Open /Libraries/Boards/MAX32670/EvKit_V1/Include/board.h and change CONSOLE_BAUD to some other value, e.g. 9600.
  3. Build again (without cleaning). Observe that no compilation occurs in the task terminal output:
    Loaded project.mk
    arm-none-eabi-size --format=berkeley <mydir>/examples/max32670_hello_world/build/max32670_hello_world.elf 
    text    data     bss     dec     hex filename
    35676    2500     628   38804    9794 <mydir>/examples/max32670_hello_world/build/max32670_hello_world.elf 
  4. Confirm via serial terminal that baud is still the original value, not the new value.

The same behavior is seen if, instead of modifying board.h, I include a new main.h file in my copy of the example and override the CONSOLE_BAUD there.

I looked into tweaking gcc.mk to address this (e.g. using an approach like this) but I could not get it to work due to lack of experience with GNU-make. I do see that the *.d dependencies are already being generated as part of the existing MSDK build approach, but it still appears as though changes to (some?) headers do not prompt recompilation via the makefile like I would have expected.

Jake-Carter commented 1 year ago

Thanks @jlj-ee, I think we're missing the inclusion of the dependency files as prerequisites for our implicit rules. I'll take a look at this

Jake-Carter commented 1 year ago

Quick update on this ticket: The dependency files seem to be working as expected with our current gcc.mk on Linux, but are failing to work on Windows.

It's getting trickier to track down the root cause of this... suspected path parsing issues on Windows, but still trying to identify the root cause.

jlj-ee commented 1 year ago

Thanks @Jake-Carter for the update. I am on Windows so that syncs with my experience. In the meantime I just clean before I build to be sure I get the latest.

Is this the implicit rule to include the dependencies?

# Include the automatically generated dependency files.
ifneq (${MAKECMDGOALS},clean)
-include ${wildcard ${BUILD_DIR}/*.d} __dummy__
endif

When I went to read about auto-dependency generation, I came across this article that uses a slightly different approach and includes an explicit rule to indicate that the object files should depend on both the source files and generated dependencies:

DEPDIR := .deps
DEPFLAGS = -MT $@ -MMD -MP -MF $(DEPDIR)/$*.d

COMPILE.c = $(CC) $(DEPFLAGS) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c

%.o : %.c
%.o : %.c $(DEPDIR)/%.d | $(DEPDIR)
        $(COMPILE.c) $(OUTPUT_OPTION) $<

$(DEPDIR): ; @mkdir -p $@

DEPFILES := $(SRCS:%.c=$(DEPDIR)/%.d)
$(DEPFILES):
include $(wildcard $(DEPFILES))

I do not see anything similar in the explicit rule for MSDK:

${BUILD_DIR}/%.o: %.c
    @if [ 'x${ECLIPSE}' != x ];                                                                                 \
    then                                                                                                        \
        echo ${CC} ${CFLAGS} -o $(call fixpath,${@}) $(call fixpath,${<}) | sed 's/-I\/\(.\)\//-I\1:\//g' ; \
    elif [ 'x${VERBOSE}' != x ];                                                                                \
    then                                                                                                        \
        echo ${CC} ${CFLAGS} -o $(call fixpath,${@}) $(call fixpath,${<});                                      \
    else                                                                                                        \
        echo "  CC    ${<}";                                                                                    \
    fi
    @${CC} ${CFLAGS} -o $(call fixpath,${@}) $(call fixpath,${<})
ifeq "$(CYGWIN)" "True"
    @sed -i -r -e 's/([A-Na-n]):/\/cygdrive\/\L\1/g' -e 's/\\([A-Za-z])/\/\1/g' ${@:.o=.d}
endif

This doesn't explain why it would work on Linux but not Windows, but would there be a benefit to adding the .d files as a dependency in the explicit rule, as in the linked article? Apologies if I am off-base; while I'm working on coming up the learning curve, my knowledge of make systems is still very rudimentary.

Jake-Carter commented 1 year ago

Yes, -include ${wildcard ${BUILD_DIR}/*.d} __dummy__ is how the dependency files are included. It's a slightly different approach than the article you linked, but I have been referencing that article as I've been troubleshooting. It's a really great implementation so thanks for sharing.

Since the existing setup works on Linux I initially guessed it was a path issue on Windows. In the dependency rules themselves I could see some malformed paths being generated like the ones below...

C:/Users/JCarter3/repos/fork/msdk/Examples/MAX78000/Hello_World/build/board.o: \
 # ...
 C:/Users/JCarter3/repos/fork/msdk/Libraries/PeriphDrivers/Include/MAX78000/gpio.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/PeriphDrivers/Include/MAX78000/mxc_pins.h \
 c:\users\jcarter3\repos\fork\msdk\libraries\miscdrivers\led\led.h \  #<-- malformed
 c:\users\jcarter3\repos\fork\msdk\libraries\miscdrivers\pushbutton\pb.h \ #<-- malformed
 C:/Users/JCarter3/repos/fork/msdk/Libraries/CMSIS/Device/Maxim/MAX78000/Include/simo_regs.h \
 c:\users\jcarter3\repos\fork\msdk\libraries\miscdrivers\display\tft_ssd2119.h \ #<-- malformed
 C:/Users/JCarter3/repos/fork/msdk/Libraries/PeriphDrivers/Include/MAX78000/spi.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/CMSIS/Device/Maxim/MAX78000/Include/spi_regs.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/PeriphDrivers/Include/MAX78000/mxc_assert.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/PeriphDrivers/Include/MAX78000/mxc_lock.h \
 c:\users\jcarter3\repos\fork\msdk\libraries\miscdrivers\touchscreen\tsc2046.h #<-- malformed

I thought the forced lower-case paths like c:\users\jcarter3\repos\fork\msdk\libraries\miscdrivers\led\led.h were causing the problems. It turns out if we tell GCC to resolve a relative path the -MD output for it will have this form, and these were coming from our BSP makefiles. I fixed that and got everything to parse correctly, but the behavior is still the same.

C:/Users/JCarter3/repos/fork/msdk/Examples/MAX78000/Hello_World/build/board.o: \
 # ...
 C:/Users/JCarter3/repos/fork/msdk/Libraries/PeriphDrivers/Include/MAX78000/gpio.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/PeriphDrivers/Include/MAX78000/mxc_pins.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/MiscDrivers/LED/led.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/MiscDrivers/PushButton/pb.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/CMSIS/Device/Maxim/MAX78000/Include/simo_regs.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/MiscDrivers/Display/tft_ssd2119.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/PeriphDrivers/Include/MAX78000/spi.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/CMSIS/Device/Maxim/MAX78000/Include/spi_regs.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/PeriphDrivers/Include/MAX78000/mxc_assert.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/PeriphDrivers/Include/MAX78000/mxc_lock.h \
 C:/Users/JCarter3/repos/fork/msdk/Libraries/MiscDrivers/Touchscreen/tsc2046.h

So now I'm porting the solution in the article you shared to our system. It's at a lower priority than some of the other work for the SDK, but hopefully this week I will have it tested. The ordering and approach from the article is slightly different, so it may reveal something we're missing in our setup. I'm still not sure why Windows specifically is failing, but MSYS2 is another factor to consider. I'll keep you posted!

Jake-Carter commented 12 months ago

@jlj-ee FYI https://github.com/Analog-Devices-MSDK/msdk/issues/528 seems to (partially) resolve this issue. The behavior is as expected on native windows as of that PR.

The problem seems to be the MSYS2 translation layer for path conversions. Still working on the right mix of /c/ and C:/-style paths inside of the .d files to get MSYS2 builds to behave

jlj-ee commented 8 months ago

Hi @Jake-Carter I have updated to the October release but still find that the build system does not detect changes if I only modify a dependent header file. Is there anything I need to change in my makefile or project.mk in order to benefit from the changes you linked a couple months ago that apparently resolved this issue on Windows?

Jake-Carter commented 8 months ago

Hi @jlj-ee

This is fixed for native Windows builds, which is an opt-in feature for this latest release. See the release notes for the Oct release: https://github.com/Analog-Devices-MSDK/msdk/releases/tag/v2023_10

I tagged this ticket to the PR in the comment above and didn't realize it auto-closed. I did not get the paths to work properly on MSYS2...

jlj-ee commented 8 months ago

Thanks for the clarification @Jake-Carter, I’ll rtfm and try again :)

Edit: Followed the instructions in the release notes to configure for using native Windows make and it works a treat. Thanks, this is a big QoL improvement!