analogdevicesinc / msdk

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

Disabling BSP integration by setting LIB_BOARD=0 leads to build error #596

Closed immanuelplattner closed 1 year ago

immanuelplattner commented 1 year ago

Dear MSDK developers,

According to the user guide you can disable the BSP integration entirely. That is exactly what I needed so I set LIB_BOARD=0 in my projects projekt.mk. After that I performed a clean and a build and got the following errors:

  *  Executing task: make -j 8 clean --output-sync=target --no-print-directory TARGET=MAX78000 BOARD=FTHR_RevA MAXIM_PATH=/home/plat/MaximSDK MAKE=make PROJECT=kws20_demo_sc

Loaded project.mk
 *  Terminal will be reused by tasks, press any key to close it. 

 *  Executing task: make -r -j 8 --output-sync=target --no-print-directory TARGET=MAX78000 BOARD=FTHR_RevA MAXIM_PATH=/home/plat/MaximSDK MAKE=make PROJECT=kws20_demo_sc 

Loaded project.mk
make -f /home/plat/MaximSDK/Libraries/PeriphDrivers/libPeriphDriver.mk  lib BUILD_DIR=/home/plat/MaximSDK/Libraries/PeriphDrivers/bin/MAX78000/softfp PROJ_CFLAGS="-Wall  -DMXC_ASSERT_ENABLE -fdiagnostics-color=always" PROJ_LDFLAGS="" MXC_OPTIMIZE_CFLAGS=-O2 MFLOAT_ABI=softfp DUAL_CORE= RISCV_CORE=
  AS    /home/plat/MaximSDK/Libraries/CMSIS/Device/Maxim/MAX78000/Source/GCC/startup_max78000.S
  CC    softmax.c
In file included from softmax.c:64:
/home/plat/MaximSDK/Libraries/PeriphDrivers/Include/MAX78000/mxc.h:45:10: fatal error: board.h: No such file or directory
   45 | #include "board.h"
      |          ^~~~~~~~~
compilation terminated.
make: *** [/home/plat/MaximSDK/Libraries/CMSIS/Device/Maxim/MAX78000/Source/GCC/gcc.mk:282: /home/plat/Documents/mt_smartcard/firmware/max78000/kws20_demo_sc/build/softmax.o] Error 1
make: *** Waiting for unfinished jobs....
  CC    cnn.c
In file included from cnn.c:55:
/home/plat/MaximSDK/Libraries/PeriphDrivers/Include/MAX78000/mxc.h:45:10: fatal error: board.h: No such file or directory
   45 | #include "board.h"
      |          ^~~~~~~~~
compilation terminated.
make: *** [/home/plat/MaximSDK/Libraries/CMSIS/Device/Maxim/MAX78000/Source/GCC/gcc.mk:282: /home/plat/Documents/mt_smartcard/firmware/max78000/kws20_demo_sc/build/cnn.o] Error 1
  CC    /home/plat/MaximSDK/Libraries/CMSIS/Device/Maxim/MAX78000/Source/heap.c
  CC    main.c
main.c:54:10: fatal error: board.h: No such file or directory
   54 | #include "board.h"
      |          ^~~~~~~~~
compilation terminated.
make: *** [/home/plat/MaximSDK/Libraries/CMSIS/Device/Maxim/MAX78000/Source/GCC/gcc.mk:281: /home/plat/Documents/mt_smartcard/firmware/max78000/kws20_demo_sc/build/main.o] Error 1
  CC    /home/plat/MaximSDK/Libraries/CMSIS/Device/Maxim/MAX78000/Source/system_max78000.c

After I got over my disappointment I had a look at the SDK header file MaximSDK/Libraries/PeriphDrivers/Include/MAX78000/mxc.h:

/******************************************************************************
 * Copyright (C) 2023 Maxim Integrated Products, Inc., All Rights Reserved.
 *
 * Permission is hereby granted, free of charge, to any person obtaining a
 * copy of this software and associated documentation files (the "Software"),
 * to deal in the Software without restriction, including without limitation
 * the rights to use, copy, modify, merge, publish, distribute, sublicense,
 * and/or sell copies of the Software, and to permit persons to whom the
 * Software is furnished to do so, subject to the following conditions:
 *
 * The above copyright notice and this permission notice shall be included
 * in all copies or substantial portions of the Software.
 *
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
 * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
 * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
 * IN NO EVENT SHALL MAXIM INTEGRATED BE LIABLE FOR ANY CLAIM, DAMAGES
 * OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
 * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
 * OTHER DEALINGS IN THE SOFTWARE.
 *
 * Except as contained in this notice, the name of Maxim Integrated
 * Products, Inc. shall not be used except as stated in the Maxim Integrated
 * Products, Inc. Branding Policy.
 *
 * The mere transfer of this software does not imply any licenses
 * of trade secrets, proprietary technology, copyrights, patents,
 * trademarks, maskwork rights, or any other form of intellectual
 * property whatsoever. Maxim Integrated Products, Inc. retains all
 * ownership rights.
 *
 ******************************************************************************/

#ifndef LIBRARIES_PERIPHDRIVERS_INCLUDE_MAX78000_MXC_H_
#define LIBRARIES_PERIPHDRIVERS_INCLUDE_MAX78000_MXC_H_

#include "mxc_device.h"
#include "mxc_delay.h"
#include "mxc_assert.h"
#include "mxc_errors.h"
#include "mxc_lock.h"
#include "mxc_pins.h"
#include "mxc_sys.h"
#include "nvic_table.h"
#include "board.h"
#include "led.h"
#ifndef BOARD_AUD01_REVA
#include "pb.h"
#endif

#ifdef BOARD_FTHR_REVA
#include "tft_ili9341.h"
#include "tsc2046.h"
#endif
#ifdef BOARD_EVKIT_V1
#include "tft_ssd2119.h"
#include "tsc2046.h"
#endif

/*
 *  Peripheral Driver Includes
 */
#include "adc.h"
#include "aes.h"
#include "cameraif.h"
#include "crc.h"
#include "dma.h"
#include "flc.h"
#include "gpio.h"
#include "i2c.h"
#include "i2s.h"
#include "icc.h"
#include "lp.h"
#include "owm.h"
#include "pt.h"
#include "rtc.h"
#include "sema.h"
#include "spi.h"
#include "tmr.h"
#include "trng.h"
#include "uart.h"
#include "wdt.h"
#include "wut.h"

#endif // LIBRARIES_PERIPHDRIVERS_INCLUDE_MAX78000_MXC_H_

After digging around in the included led.h and pb.h I propose to change mxc.h so something like this:

#ifndef LIBRARIES_PERIPHDRIVERS_INCLUDE_MAX78000_MXC_H_
#define LIBRARIES_PERIPHDRIVERS_INCLUDE_MAX78000_MXC_H_

#include "mxc_device.h"
#include "mxc_delay.h"
#include "mxc_assert.h"
#include "mxc_errors.h"
#include "mxc_lock.h"
#include "mxc_pins.h"
#include "mxc_sys.h"
#include "nvic_table.h"

// Check if BSP integration is enabled
#if LIB_BOARD == 1
#include "board.h"
#include "led.h"
#ifndef BOARD_AUD01_REVA
#include "pb.h"
#endif
#ifdef BOARD_FTHR_REVA
#include "tft_ili9341.h"
#include "tsc2046.h"
#endif
#ifdef BOARD_EVKIT_V1
#include "tft_ssd2119.h"
#include "tsc2046.h"
#endif
#endif // LIB_BOARD == 1

/*
 *  Peripheral Driver Includes
 */
#include "adc.h"
#include "aes.h"
#include "cameraif.h"
#include "crc.h"
#include "dma.h"
#include "flc.h"
#include "gpio.h"
#include "i2c.h"
#include "i2s.h"
#include "icc.h"
#include "lp.h"
#include "owm.h"
#include "pt.h"
#include "rtc.h"
#include "sema.h"
#include "spi.h"
#include "tmr.h"
#include "trng.h"
#include "uart.h"
#include "wdt.h"
#include "wut.h"

#endif // LIBRARIES_PERIPHDRIVERS_INCLUDE_MAX78000_MXC_H_

That way, one can truly deactivate the BSP in one fell swoop. As it currently is, I am forced to define a header file called board.h somewhere, because it is obviously a bad idea to change a header file in the SDK, which is also used in a number of other projects.

I also take issue with the official approach to custom BSPs as detailed in the user guide. This section instructs the user to create custom BSPs in a certain directory in the SDK. I personally prefer to not touch the SDK at all. Because as soon as you do, you basically have to store the whole changed SDK in your software project. It is also very annoying/difficult to keep track of all the changes you make. Very quickly something falls through the cracks and that can lead to issues down the line. In short: It is a disaster in terms of reproducibility.

That said, I want to say thank you that you even have such instructions. Unfortunately, a lot of SDKs have way worse documentation. They basically force you to browse through the whole file structure and hunt through all the make files and piece it together yourself. So thank you for that.

Regards

immanuelplattner commented 1 year ago

I discovered it gets even worse. The mxc.h includes also the header files led.h and pb.h. So you have to define them also somewhere or use the generic drivers in MaximSDK/Libraries/MiscDrivers/LED and MaximSDK/Libraries/MiscDrivers/PushButton. But that is even more work, because you need to add them in the make file and also you need to define certain global variables.

My current project.mk:

# This file can be used to set build configuration
# variables.  These variables are defined in a file called 
# "Makefile" that is located next to this one.

# For instructions on how to use this system, see
# https://analog-devices-msdk.github.io/msdk/USERGUIDE/#build-system

#MXC_OPTIMIZE_CFLAGS = -Og
# ^ For example, you can uncomment this line to 
# optimize the project for debugging

################################################################################

MXC_OPTIMIZE_CFLAGS = -O2

# Setting the target chip
TARGET = MAX78000

# Since we are working with our custum hardware, we do not load any Maxim board
# specific libraries.
LIB_BOARD = 0
# Where to find the missing BSP source files (included in mxc.h)
VPATH += $(MAXIM_PATH)/Libraries/MiscDrivers/LED
VPATH += $(MAXIM_PATH)/Libraries/MiscDrivers/PushButton
# Where to find the missing BSP header files (include in mxc.h)
IPATH += $(MAXIM_PATH)/Libraries/MiscDrivers/LED
IPATH += $(MAXIM_PATH)/Libraries/MiscDrivers/PushButton

# If enabled, the mic samples used for inference are sent to the serial port
#PROJ_CFLAGS+=-DSEND_MIC_OUT_SERIAL

My current *board.h":

/*******************************************************************************
 * This file is necessary, because the SDK file
 * MaximSDK/Libraries/PeriphDrivers/Include/MAX78000/mxc.h includes a board.h
 * file on line 45 and the author did not want to change a header file in the
 * SDK.
 ******************************************************************************/
#ifndef __BOARD_H__
#define __BOARD_H__

#define BOARD_SMARTCARD_V01
#define CONSOLE_UART    0
#define CONSOLE_BAUD    115200

#endif // __BOARD_H__

And my current board.c:

#include "board.h"
#include "gpio.h"

// Definition of the LED pins (for MaximSDK/Libraries/MiscDrivers/LED)
const mxc_gpio_cfg_t led_pin[] = {};    // No LEDs on the hardware
const unsigned int num_leds = (sizeof(led_pin) / sizeof(mxc_gpio_cfg_t));
// Definition of the buttons pins (for MaximSDK/Libraries/MiscDrivers/PushButton)
const mxc_gpio_cfg_t pb_pin[] = {};     // No buttons on the hardware
const unsigned int num_pbs = (sizeof(pb_pin) / sizeof(mxc_gpio_cfg_t));

Now the project compiles fine. I just think it is a rather big effort when it should have been just LIB_BOARD = 0.

Jake-Carter commented 1 year ago

Thanks for your feedback @immanuelplattner

mxc.h is really the "include everything" header file, but I agree that we could decouple it from the BSP better as you've suggested.

With LIB_BOARD = 0 getting a missing inclusion error on "board.h" is somewhat intended behavior, since the board library is the one responsible for providing that file. Even for custom BSPs we envision LIB_BOARD = 1, but this is a suggestion and not a requirement.

With that being said we could allow users to set BOARD_DIR to help move the search location of the BSP out of the SDK. It's currently hard set in libs.mk here and changing it to an overridable variable would be an easy update.

After enabling this, project.mk would could then look something like

# This file can be used to set build configuration
# variables.  These variables are defined in a file called 
# "Makefile" that is located next to this one.

# For instructions on how to use this system, see
# https://analog-devices-msdk.github.io/msdk/USERGUIDE/#build-system

#MXC_OPTIMIZE_CFLAGS = -Og
# ^ For example, you can uncomment this line to 
# optimize the project for debugging

################################################################################

MXC_OPTIMIZE_CFLAGS = -O2

# Setting the target chip
TARGET = MAX78000

# Load a custom bsp from ./path/to/your/bspfolder/yourbspname
# This expects a "board.mk" file at ./path/to/your/bspfolder/yourbspname/board.mk
BOARD_DIR = ./path/to/your/bspfolder
BOARD = yourbspname

# If enabled, the mic samples used for inference are sent to the serial port
#PROJ_CFLAGS+=-DSEND_MIC_OUT_SERIAL

and the "board.mk" file for your bsp would look something like

# Where to find the missing BSP source files (included in mxc.h)
VPATH += $(MAXIM_PATH)/Libraries/MiscDrivers/LED
VPATH += $(MAXIM_PATH)/Libraries/MiscDrivers/PushButton
# Where to find the missing BSP header files (include in mxc.h)
IPATH += $(MAXIM_PATH)/Libraries/MiscDrivers/LED
IPATH += $(MAXIM_PATH)/Libraries/MiscDrivers/PushButton
# .. Other BSP setup...

However, this is only useful if your board does actually need the LED/PushButton, drivers, etc. If that is not the case, and your board setup code is entirely contained in your application code, then I see why LIB_BOARD = 0 is attractive.

So there are two components we will implement:

  1. Ensure mxc.h is safe against LIB_BOARD = 0
  2. Allow users to override BOARD_DIR so that BSPs can be created outside of the MSDK.
immanuelplattner commented 1 year ago

Thanks @Jake-Carter for the fast response.

I think that the two changes you are planning are generally beneficial for MSDK users. I would also sugest that LEDs and buttons are not a must when one wants to use the BSP integration (not contained in my header.h change proposal). E. g. in my case the hardware I am currently writing code for does not have LEDs nor buttons. I am also not a big fan of "include it all" header files. It can enlarge the code unnecessarily and it obfuscates the real dependencies. But that is just my opinion.

Jake-Carter commented 1 year ago

@immanuelplattner I've addressed the suggested changes in the PR above. Your review/testing is more than welcome

immanuelplattner commented 1 year ago

@Jake-Carter Thanks for the heads-up. I generally like what you did. I think the changes you made to be good ones. I also agree with @ozersa that "First I believe the board.h file shall not be include by drivers include file, X32xxx.h or mxc.h. It shall not be depend on the board related includes.".