btashton commented on a change in pull request #2735:
URL: https://github.com/apache/incubator-nuttx/pull/2735#discussion_r563327349



##########
File path: arch/arm/src/nrf52/Make.defs
##########
@@ -163,3 +163,89 @@ endif
 ifeq ($(CONFIG_PM),y)
 CHIP_CSRCS += nrf52_pminitialize.c
 endif
+
+ifeq ($(CONFIG_NRF52_SOFTDEVICE_CONTROLLER),y)
+
+NRFXLIB_UNPACK  := sdk-nrfxlib
+NRFXLIB_VER     := 1.4.2
+NRFXLIB_REF     := v$(NRFXLIB_VER)
+NRFXLIB_TGZ     := $(NRFXLIB_REF).tar.gz
+NRFXLIB_URL     := https://github.com/nrfconnect/sdk-nrfxlib/archive
+
+NRFX_UNPACK     := nrfx
+NRFX_VER        := 2.4.0
+NRFX_REF        := v$(NRFX_VER)
+NRFX_TGZ        := $(NRFX_REF).tar.gz
+NRFX_URL        := https://github.com/NordicSemiconductor/nrfx/archive
+
+CMSIS_UNPACK    := CMSIS_5
+CMSIS_VER       := 5.7.0
+CMSIS_REF       := $(CMSIS_VER)
+CMSIS_TGZ       := $(CMSIS_REF).tar.gz
+CMSIS_URL       := https://github.com/ARM-software/CMSIS_5/archive
+
+$(NRFXLIB_TGZ):
+       $(Q) echo "Downloading: NRFXLIB"
+       $(Q) curl -L $(NRFXLIB_URL)/$(NRFXLIB_TGZ) -o chip/$(NRFXLIB_TGZ)
+
+$(NRFX_TGZ):
+       $(Q) echo "Downloading: NRFX"
+       $(Q) curl -L $(NRFX_URL)/$(NRFX_TGZ) -o chip/$(NRFX_TGZ)
+
+$(CMSIS_TGZ):
+       $(Q) echo "Downloading: CMSIS"
+       $(Q) curl -L $(CMSIS_URL)/$(CMSIS_TGZ) -o chip/$(CMSIS_TGZ)
+
+chip/$(NRFXLIB_UNPACK): $(NRFXLIB_TGZ)
+       $(Q) echo "Unpacking: NRXFLIB"
+       $(Q) cd chip && tar zxf $(NRFXLIB_TGZ)
+       $(Q) mv chip/$(NRFXLIB_UNPACK)-$(NRFXLIB_VER)* chip/$(NRFXLIB_UNPACK)
+       $(Q) touch chip/$(NRFXLIB_UNPACK)
+
+chip/$(NRFX_UNPACK): $(NRFX_TGZ)
+       $(Q) echo "Unpacking: NRXF"
+       $(Q) cd chip && tar zxf $(NRFX_TGZ)
+       $(Q) mv chip/$(NRFX_UNPACK)-$(NRFX_VER)* chip/$(NRFX_UNPACK)
+       $(Q) touch chip/$(NRFX_UNPACK)
+
+chip/$(CMSIS_UNPACK): $(CMSIS_TGZ)

Review comment:
       I'm super uncomfortable with this, this is pulling in an entire new HAL 
for ARM. I don't think we actually need to do this. We should be able to just 
stub any needed interfaces for the libraries. 

##########
File path: arch/arm/src/nrf52/Make.defs
##########
@@ -163,3 +163,89 @@ endif
 ifeq ($(CONFIG_PM),y)
 CHIP_CSRCS += nrf52_pminitialize.c
 endif
+
+ifeq ($(CONFIG_NRF52_SOFTDEVICE_CONTROLLER),y)
+
+NRFXLIB_UNPACK  := sdk-nrfxlib
+NRFXLIB_VER     := 1.4.2
+NRFXLIB_REF     := v$(NRFXLIB_VER)
+NRFXLIB_TGZ     := $(NRFXLIB_REF).tar.gz
+NRFXLIB_URL     := https://github.com/nrfconnect/sdk-nrfxlib/archive
+
+NRFX_UNPACK     := nrfx
+NRFX_VER        := 2.4.0
+NRFX_REF        := v$(NRFX_VER)
+NRFX_TGZ        := $(NRFX_REF).tar.gz
+NRFX_URL        := https://github.com/NordicSemiconductor/nrfx/archive
+
+CMSIS_UNPACK    := CMSIS_5
+CMSIS_VER       := 5.7.0
+CMSIS_REF       := $(CMSIS_VER)
+CMSIS_TGZ       := $(CMSIS_REF).tar.gz
+CMSIS_URL       := https://github.com/ARM-software/CMSIS_5/archive
+
+$(NRFXLIB_TGZ):
+       $(Q) echo "Downloading: NRFXLIB"
+       $(Q) curl -L $(NRFXLIB_URL)/$(NRFXLIB_TGZ) -o chip/$(NRFXLIB_TGZ)
+
+$(NRFX_TGZ):

Review comment:
       What do we actually need out of here that we cannot supply ourselves.  
This is pulling in the whole nrf HAL

##########
File path: arch/arm/src/nrf52/Make.defs
##########
@@ -163,3 +163,89 @@ endif
 ifeq ($(CONFIG_PM),y)
 CHIP_CSRCS += nrf52_pminitialize.c
 endif
+
+ifeq ($(CONFIG_NRF52_SOFTDEVICE_CONTROLLER),y)
+
+NRFXLIB_UNPACK  := sdk-nrfxlib
+NRFXLIB_VER     := 1.4.2
+NRFXLIB_REF     := v$(NRFXLIB_VER)
+NRFXLIB_TGZ     := $(NRFXLIB_REF).tar.gz
+NRFXLIB_URL     := https://github.com/nrfconnect/sdk-nrfxlib/archive
+
+NRFX_UNPACK     := nrfx
+NRFX_VER        := 2.4.0
+NRFX_REF        := v$(NRFX_VER)
+NRFX_TGZ        := $(NRFX_REF).tar.gz
+NRFX_URL        := https://github.com/NordicSemiconductor/nrfx/archive
+
+CMSIS_UNPACK    := CMSIS_5
+CMSIS_VER       := 5.7.0
+CMSIS_REF       := $(CMSIS_VER)
+CMSIS_TGZ       := $(CMSIS_REF).tar.gz
+CMSIS_URL       := https://github.com/ARM-software/CMSIS_5/archive
+
+$(NRFXLIB_TGZ):

Review comment:
       This library seems like the only one we actually need and we should be 
able to only pull the lib files out. 

##########
File path: arch/arm/src/nrf52/Make.defs
##########
@@ -163,3 +163,89 @@ endif
 ifeq ($(CONFIG_PM),y)
 CHIP_CSRCS += nrf52_pminitialize.c
 endif
+
+ifeq ($(CONFIG_NRF52_SOFTDEVICE_CONTROLLER),y)
+
+NRFXLIB_UNPACK  := sdk-nrfxlib
+NRFXLIB_VER     := 1.4.2
+NRFXLIB_REF     := v$(NRFXLIB_VER)
+NRFXLIB_TGZ     := $(NRFXLIB_REF).tar.gz
+NRFXLIB_URL     := https://github.com/nrfconnect/sdk-nrfxlib/archive
+
+NRFX_UNPACK     := nrfx
+NRFX_VER        := 2.4.0
+NRFX_REF        := v$(NRFX_VER)
+NRFX_TGZ        := $(NRFX_REF).tar.gz
+NRFX_URL        := https://github.com/NordicSemiconductor/nrfx/archive
+
+CMSIS_UNPACK    := CMSIS_5
+CMSIS_VER       := 5.7.0
+CMSIS_REF       := $(CMSIS_VER)
+CMSIS_TGZ       := $(CMSIS_REF).tar.gz
+CMSIS_URL       := https://github.com/ARM-software/CMSIS_5/archive
+
+$(NRFXLIB_TGZ):
+       $(Q) echo "Downloading: NRFXLIB"
+       $(Q) curl -L $(NRFXLIB_URL)/$(NRFXLIB_TGZ) -o chip/$(NRFXLIB_TGZ)
+
+$(NRFX_TGZ):
+       $(Q) echo "Downloading: NRFX"
+       $(Q) curl -L $(NRFX_URL)/$(NRFX_TGZ) -o chip/$(NRFX_TGZ)
+
+$(CMSIS_TGZ):
+       $(Q) echo "Downloading: CMSIS"
+       $(Q) curl -L $(CMSIS_URL)/$(CMSIS_TGZ) -o chip/$(CMSIS_TGZ)
+
+chip/$(NRFXLIB_UNPACK): $(NRFXLIB_TGZ)
+       $(Q) echo "Unpacking: NRXFLIB"
+       $(Q) cd chip && tar zxf $(NRFXLIB_TGZ)
+       $(Q) mv chip/$(NRFXLIB_UNPACK)-$(NRFXLIB_VER)* chip/$(NRFXLIB_UNPACK)
+       $(Q) touch chip/$(NRFXLIB_UNPACK)
+
+chip/$(NRFX_UNPACK): $(NRFX_TGZ)
+       $(Q) echo "Unpacking: NRXF"
+       $(Q) cd chip && tar zxf $(NRFX_TGZ)
+       $(Q) mv chip/$(NRFX_UNPACK)-$(NRFX_VER)* chip/$(NRFX_UNPACK)
+       $(Q) touch chip/$(NRFX_UNPACK)
+
+chip/$(CMSIS_UNPACK): $(CMSIS_TGZ)

Review comment:
       But why not just provide our header and only use the lib files. Then we 
avoid having to pull in the other two libraries?

##########
File path: arch/arm/src/nrf52/Make.defs
##########
@@ -163,3 +163,89 @@ endif
 ifeq ($(CONFIG_PM),y)
 CHIP_CSRCS += nrf52_pminitialize.c
 endif
+
+ifeq ($(CONFIG_NRF52_SOFTDEVICE_CONTROLLER),y)
+
+NRFXLIB_UNPACK  := sdk-nrfxlib
+NRFXLIB_VER     := 1.4.2
+NRFXLIB_REF     := v$(NRFXLIB_VER)
+NRFXLIB_TGZ     := $(NRFXLIB_REF).tar.gz
+NRFXLIB_URL     := https://github.com/nrfconnect/sdk-nrfxlib/archive
+
+NRFX_UNPACK     := nrfx
+NRFX_VER        := 2.4.0
+NRFX_REF        := v$(NRFX_VER)
+NRFX_TGZ        := $(NRFX_REF).tar.gz
+NRFX_URL        := https://github.com/NordicSemiconductor/nrfx/archive
+
+CMSIS_UNPACK    := CMSIS_5
+CMSIS_VER       := 5.7.0
+CMSIS_REF       := $(CMSIS_VER)
+CMSIS_TGZ       := $(CMSIS_REF).tar.gz
+CMSIS_URL       := https://github.com/ARM-software/CMSIS_5/archive
+
+$(NRFXLIB_TGZ):
+       $(Q) echo "Downloading: NRFXLIB"
+       $(Q) curl -L $(NRFXLIB_URL)/$(NRFXLIB_TGZ) -o chip/$(NRFXLIB_TGZ)
+
+$(NRFX_TGZ):
+       $(Q) echo "Downloading: NRFX"
+       $(Q) curl -L $(NRFX_URL)/$(NRFX_TGZ) -o chip/$(NRFX_TGZ)
+
+$(CMSIS_TGZ):
+       $(Q) echo "Downloading: CMSIS"
+       $(Q) curl -L $(CMSIS_URL)/$(CMSIS_TGZ) -o chip/$(CMSIS_TGZ)
+
+chip/$(NRFXLIB_UNPACK): $(NRFXLIB_TGZ)
+       $(Q) echo "Unpacking: NRXFLIB"
+       $(Q) cd chip && tar zxf $(NRFXLIB_TGZ)
+       $(Q) mv chip/$(NRFXLIB_UNPACK)-$(NRFXLIB_VER)* chip/$(NRFXLIB_UNPACK)
+       $(Q) touch chip/$(NRFXLIB_UNPACK)
+
+chip/$(NRFX_UNPACK): $(NRFX_TGZ)
+       $(Q) echo "Unpacking: NRXF"
+       $(Q) cd chip && tar zxf $(NRFX_TGZ)
+       $(Q) mv chip/$(NRFX_UNPACK)-$(NRFX_VER)* chip/$(NRFX_UNPACK)
+       $(Q) touch chip/$(NRFX_UNPACK)
+
+chip/$(CMSIS_UNPACK): $(CMSIS_TGZ)

Review comment:
       I'm only talking about the interfaces that we need for this that should 
be a quite small surface. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Reply via email to