Update to allow mbed and the mbed online compiler to build. #449

GeorgeWort · 2019-07-03T13:15:34Z

Update to allow mbed and the mbed online compiler to build.
This allows gcc_arm, armcc, and armclang to build the DAL.

Most changes are my personal changes in order to comply with the restrictions of the up to date compilers, and are meant to give equivalent behaviour to previously.

microbit-sam · 2019-07-03T13:16:56Z

Linked to:
nrf51-sdk: lancaster-university/nrf51-sdk#1
mbed-classic: lancaster-university/mbed-classic#13
microbit-dal: #449

ghost · 2019-07-03T13:30:58Z

Not looked at the detail but I see quite a lot of files have changed and I'm wondering what regression testing has been performed / will be performed? I note the changes are "meant to give equivalent behaviour".... but would feel reassured if this had been proven. Thanks.

martinwork · 2019-07-03T14:31:46Z

In places, this appears to be a development from an earlier version of the DAL. It looks like that's the only reason for the MicroBitBLEManager changes, for example.
Other files are purely white space changes.
Should we be adding .lib files? If they are necessary, maybe this should be in a branch or fork repo.
Will it still compile with yotta?
Why replace temporary arrays on the stack with new/delete heap allocations?
It's hard to make out what the essential changes are.

microbit-carlos · 2019-07-03T13:54:42Z

source/types/MicroBitImage.cpp

@@ -854,7 +858,7 @@ MicroBitImage MicroBitImage::crop(int startx, int starty, int cropWidth, int cro
        newHeight = getHeight();

    //allocate our storage.
-    uint8_t cropped[newWidth * newHeight];
+    uint8_t *cropped = new uint8_t[newWidth * newHeight];


What's the reason this and other buffers have been moved from stack to the heap?

The updated versions of all compilers will not allocate a dynamically sized array on the stack, expression must have a constant value. Though looking at this I have changed the type without changing how it is used, which can't be right.

These are the main things I would want to test thoroughly.

Is that only on armcc5? Might be another C++11 feature.

microbit-carlos · 2019-07-03T13:55:44Z

source/drivers/MicroBitQuadratureDecoder.cpp

@@ -205,7 +212,7 @@ void MicroBitQuadratureDecoder::poll()
 {
    NRF_QDEC->TASKS_READCLRACC = 1;
    position += (int32_t)NRF_QDEC->ACCREAD;
-    faults = min(UINT16_MAX, faults + NRF_QDEC->ACCDBLREAD);
+    faults = min(0xFFFF, faults + NRF_QDEC->ACCDBLREAD);


Is UINT16_MAX not provided by the other compilers? It should be part of stdint.h, no?
If not, can we check if it's not defined and define it?

It doesn't appear to be for the armc5 compiler no. Where would be best to define it?

Top of the file I guess, we can move it somewhere else if somebody else requests it.

microbit-carlos · 2019-07-03T14:04:00Z

source/drivers/MicroBitCompassCalibrator.cpp

-    Sample3D centre = { 0,0,0 };
-    Sample3D best = { 0,0,0 };
+    Sample3D centre;
+    Sample3D best;


The default constructor initialises the values to zero, but is there any specific reason to remove the {0,0,0}?
Also, to reduce the diff as much as possible, can we undo all the whitespace character changes?

Same as the other initializer removals. I will remove the whitespace.

microbit-carlos · 2019-07-03T14:05:01Z

source/core/MicroBitHeapAllocator.cpp

@@ -59,7 +59,13 @@ DEALINGS IN THE SOFTWARE.
 // A list of all active heap regions, and their dimensions in memory.
 HeapDefinition heap[MICROBIT_MAXIMUM_HEAPS] = { };
 uint8_t heap_count = 0;
-extern "C" int __end__;
+#if defined __CC_ARM || defined __ARMCC_VERSION
+#define HEAP_START Image$$RW_IRAM1$$ZI$$Limit


I have no experience with armcc, but this is a really weird-looking and cryptic symbol, is there perhaps a better one for this value?

I do not know, I extracted this symbol from https://github.com/lancaster-university/mbed-classic/blob/master/targets/cmsis/TARGET_Freescale/TARGET_K20XX/TARGET_K20D50M/TOOLCHAIN_ARM_STD/sys.cpp.

I can see this is also defined here: https://github.com/lancaster-university/mbed-classic/blob/master/targets/cmsis/TARGET_NORDIC/TARGET_MCU_NRF51822/TOOLCHAIN_ARM_STD/sys.cpp#L15

As weird looking at it seems, I guess it is what it is ¯\_(ツ)_/¯

microbit-carlos · 2019-07-03T14:05:54Z

source/bluetooth/MicroBitPartialFlashingService.cpp

+    packetCount = 0;
+    blockPacketCount = 0;
+    blockNum = 0;
+    offset   = 0;


Is this necessary for the compiler compatibility?

Same as others.

microbit-carlos · 2019-07-03T14:06:41Z

source/bluetooth/MicroBitBLEManager.cpp

@@ -242,6 +243,7 @@ MicroBitBLEManager::MicroBitBLEManager(MicroBitStorage &_storage) : storage(&_st
 */
 MicroBitBLEManager::MicroBitBLEManager() : storage(NULL)
 {
+    currentMode = MICROBIT_MODE_APPLICATION;


If this is a bug fix, could you submit it as a different PR?
Same for inc/bluetooth/MicroBitBLEManager.h.

It is not a bug fix, it is to compensate for the removal of the initializer due to Error: #321: data member initializer is not allowed from the armc5 compiler.

microbit-carlos · 2019-07-03T14:19:34Z

source/asm/TOOLCHAIN_GCC_ARM/CortexContextSwitch.s

@@ -0,0 +1,292 @@
+@ The MIT License (MIT)


Is this file meant to replace https://github.com/lancaster-university/microbit-dal/blob/master/source/asm/CortexContextSwitch.s.gcc? If so should the old one be removed?

Same as previous.

So the makefile needs the source/asm/CortexContextSwitch.s.gcc as well?

Right now we have two GCC asm files:

source/asm/CortexContextSwitch.s.gcc

source/asm/TOOLCHAIN_GCC_ARM/CortexContextSwitch.s

So that it can copy one of them to source/asm/CortexContextSwitch.s?
How does it decide which one of the two to copy when using GCC?

When using the makefile (yotta?) it ignored the TOOLCHAIN_* directory and checks the compiler flags to copy the correct .s.gcc/armcc file to .s.

Where is this makefile or what tool/steps generates it?

https://github.com/lancaster-university/microbit-dal/blob/master/source/CMakeLists.txt

Thanks!
So it looks like we are manually maintaining that CMake file anyway, and in that case we can move the original GCC and ARMCC assembly files to the new location required by mbed-cli, and update these lines to find them:

microbit-dal/source/CMakeLists.txt

Lines 88 to 94 in 8c2403e

if(CMAKE_COMPILER_IS_GNUCC)

file(REMOVE "asm/CortexContextSwitch.s")

configure_file("asm/CortexContextSwitch.s.gcc" "asm/CortexContextSwitch.s" COPYONLY)

else()

file(REMOVE "asm/CortexContextSwitch.s")

configure_file("asm/CortexContextSwitch.s.armcc" "asm/CortexContextSwitch.s" COPYONLY)

endif()

microbit-carlos · 2019-07-03T14:21:01Z

source/asm/TOOLCHAIN_ARM_STD/CortexContextSwitch.S

@@ -0,0 +1,293 @@
+; The MIT License (MIT)


Is this file meant to replace https://github.com/lancaster-university/microbit-dal/blob/master/source/asm/CortexContextSwitch.s.armcc ? If not, can CortexContextSwitch.s.armcc be moved to a subdirectory similar to this new file path pattern?

The makefile requires that file to exist there so that it can select which file to copy to asm/CortexContextSwitch.s at compile time.

microbit-carlos · 2019-07-03T14:24:09Z

inc/drivers/MicroBitQuadratureDecoder.h

-    uint8_t         LEDDelay = 0;       // power-up time for LED, in microseconds
+    uint32_t        samplePeriod;       // Minimum sampling period allowed
+    uint16_t        faults;             // Double-transition counter
+    uint8_t         LEDDelay;           // power-up time for LED, in microseconds


If these values are set in the constructor, do we need to set them here as well? If not I would rather reduce this PR as much as possible.

I have set them in the constructor because I have removed them here due to the armc5 compiler error: Error: #321: data member initializer is not allowed.

microbit-carlos · 2019-07-03T14:34:05Z

BLE_API.lib

@@ -0,0 +1 @@
+https://github.com/lancaster-university/BLE_API/#bbc2dc58f8da1d5a6d1882dddd5ce7399dc48889


I assume these lib files are now needed for mbed-cli, is there any way around them?
If not, we now have two sources of truth, the yotta json file and these individual lib files.
What can we do to keep these in sync?
Can these lib files use git tags instead of commit hashes?

They do not need to be inside the microbit-dal repository at all, they can be in the microbit repository or pulled in manually.
I tried using a git tag and got ERROR: Named branches not allowed in .lib.

Right but they have to be "somewhere", no?
If pulled manually does that mean extra steps when setting up the workspace?
I wouldn't mind having a script that parses the dependencies from the yotta json file and executes the right mbed-cli commands when setting up the workspace, what do you think the best place for a script like that would be?

Somewhere yes.
Extra steps would be needed.
A script could extract the dependencies and use mbed add followed by mbed deploy. Best place would have to be root of this repository unless we introduce a tools directory.

jamesadevine · 2019-07-03T14:43:56Z

I'm a bit nervous about most of these changes. I think these will require extensive regression tests. @jaustin

GeorgeWort · 2019-07-03T16:46:30Z

The scope of this PR will be reduced to only include the changes required for ARMC6 compatibility.

martinwork · 2019-07-03T17:08:44Z

Could the changes for each syntax compatibility issue be grouped, so we can record what syntax to avoid in the future? Maybe a separate PR for the changes needed for the build system?

error: default initialization of an object of const type 'const KeyValueTable' without a user-provided default constructor.

error: non-aggregate type 'Sample3D' cannot be initialized with an initializer list.

microbit-carlos · 2019-07-05T12:32:12Z

source/CMakeLists.txt

-  file(REMOVE "asm/CortexContextSwitch.s")
-  configure_file("asm/CortexContextSwitch.s.armcc" "asm/CortexContextSwitch.s" COPYONLY)
-endif()
-


Doesn't this need to be updated to move the asm/TOOLCHAIN_GCC_ARM/CortexContextSwitch.s or asm/TOOLCHAIN_ARM_STD/CortexContextSwitch.S into asm/CortexContextSwitch.s?

Yotta appears to pay attention to TOOLCHAIN_* directories, as there are plenty in mbed-classic which cause no conflicts, though I do need to modify the line below to point at the new location.

So in summary, yes.

microbit-carlos · 2019-07-05T12:33:48Z

source/drivers/MicroBitCompassCalibrator.cpp

-    Sample3D centre = { 0,0,0 };
-    Sample3D best = { 0,0,0 };
+    Sample3D centre;
+    Sample3D best;


If the constructor code has been reverted (was there constructor code to initialise these to 0?), shouldn't these 3 lines be reverted as well?

There wasn't constructor code to initialise these as they are initialised to zero by default.

@geowor01 I don't think there are any guarantees that struct fields are zero initialised unless they are placed in bss (globals).

@jamesadevine The default constructor sets the value to zero.

microbit-dal/inc/types/CoordinateSystem.h

Line 81 in 8c2403e

this->x = 0;

@geowor01 Ah cool 😄, my bad!

GeorgeWort · 2019-07-05T13:41:15Z

Commits 5b015a3 and 4f1a31f are enough to allow mbed-cli to work with gcc-arm.

This was referenced Jul 3, 2019

Update for ARMCC6 lancaster-university/mbed-classic#13

Draft

Support for ARMCC6 lancaster-university/nrf51-sdk#1

Draft

microbit-carlos reviewed Jul 3, 2019

View reviewed changes

GeorgeWort added 3 commits July 5, 2019 12:26

Implement MicroBitHeapAllocator without relying on gcc malloc hooks.

7bf0c72

Assign to CREATE_KEY_VALUE_TABLE due to the following error.

f2baf1e

error: default initialization of an object of const type 'const KeyValueTable' without a user-provided default constructor.

Remove initializer lists due the following error.

5b015a3

error: non-aggregate type 'Sample3D' cannot be initialized with an initializer list.

GeorgeWort force-pushed the update-dal branch from 9d05b48 to 82eafc6 Compare July 5, 2019 12:26

microbit-carlos reviewed Jul 5, 2019

View reviewed changes

Rename and move CortexContextSwitch files.

4f1a31f

GeorgeWort force-pushed the update-dal branch from 82eafc6 to 4f1a31f Compare July 5, 2019 13:33

	if(CMAKE_COMPILER_IS_GNUCC)
	file(REMOVE "asm/CortexContextSwitch.s")
	configure_file("asm/CortexContextSwitch.s.gcc" "asm/CortexContextSwitch.s" COPYONLY)
	else()
	file(REMOVE "asm/CortexContextSwitch.s")
	configure_file("asm/CortexContextSwitch.s.armcc" "asm/CortexContextSwitch.s" COPYONLY)
	endif()

		@@ -0,0 +1 @@
		https://github.com/lancaster-university/BLE_API/#bbc2dc58f8da1d5a6d1882dddd5ce7399dc48889

Update to allow mbed and the mbed online compiler to build. #449

Are you sure you want to change the base?

Update to allow mbed and the mbed online compiler to build. #449

Conversation

GeorgeWort commented Jul 3, 2019

microbit-sam commented Jul 3, 2019

ghost commented Jul 3, 2019

martinwork commented Jul 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeWort Jul 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeWort Jul 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeWort Jul 3, 2019 • edited Loading

Choose a reason for hiding this comment

jamesadevine commented Jul 3, 2019

GeorgeWort commented Jul 3, 2019

martinwork commented Jul 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeWort commented Jul 5, 2019

GeorgeWort Jul 3, 2019 •

edited

Loading

GeorgeWort Jul 3, 2019 •

edited

Loading

GeorgeWort Jul 3, 2019 •

edited

Loading