provided a script for moving from device.h to target.json

[theotherjimmy]

use -h to find out how it works!

also auto-formatted hal/target.json

cc @screamerbg

[theotherjimmy]

example diff after running python workspace_tools/remove-device-h.py -t LPC23XX

diff --git a/hal/targets.json b/hal/targets.json
index c3ad9a1..747c3ed 100644
--- a/hal/targets.json
+++ b/hal/targets.json
@@ -590,6 +610,27 @@
  "LPC2368": {
        "inherits": [
            "LPCTarget"
        ],
        "core": "ARM7TDMI-S",
        "progen": {
            "target": "lpc2368"
        },
        "extra_labels": [
            "NXP",
            "LPC23XX"
        ],
        "supported_toolchains": [
             "ARM",
             "GCC_ARM",
             "GCC_CR"
+        ],
+        "provides": [
+            "LOCALFILESYSTEM",
+            "ANALOGIN",
+            "RTC",
+            "SPI",
+            "SEMIHOST",
+            "INTERRUPTIN",
+            "ERROR_PATTERN",
+            "PORTINOUT",
+            "PORTIN",
+            "STDIO_MESSAGES",
+            "I2CSLAVE",
+            "PWMOUT",
+            "CAN",
+            "SPISLAVE",
+            "PORTOUT",
+            "ETHERNET",
+            "SERIAL",
+            "I2C",
+            "ANALOGOUT"
         ]
     },

[0xc0170]

I am confused. Is the one commit about changing formating in targets.json? that should be separate commit.

Do we need this script in this code base ? I assume we do device.h to json, generate that header file once for all target, and that's it?

[theotherjimmy]

I'll amend to separate the commits.

[theotherjimmy]

@0xc0170 done rebasing. I was committing it so that if someone decides to make a device.h in the future, we can easily convert it.

[screamerbg]

This is awesome! Now we have unified target definition that propagates to device features! Also I like the variable order consistency (e.g. inherits is always first).
2 small suggestions:

• alphabetic sort values in "provides"
• can we reduce the multiline targets.json?

[sg-]

👍

[bogdanm]

Looks nice, however:

1. This can't really be accepted without removing the corresponding device.h files. Otherwise you'll get loads of warnings related to multiple macro definitions.
2. It results in a very large target.json files. In targets.py, targets can already inherit macros from their base target(s) and change them using macros_add and macros_remove. This can be extended to provides and the result would likely be an important reduction in size for targets.json.
3. To keep the number of lines in targets.json under control, targets.json lists small arrays in a single line, see for example https://github.com/mbedmicro/mbed/blob/master/hal/targets.json#L50. Please keep this convention at least for extra_labels, macros and supported_toolchains`.
4. Are there any tests than can prove that the script correctly interprets all the device.h files in the tree? If it doesn't, we're going to get invalid builds, and that's going to be hard to diagnose.

[bogdanm]

Point 2 above is especially imporant, since the current targets implementation allows target composition (building a board target on top of a MCU target, for example). If we just define the provides field in each target, we give up on a large part of this, which would be a pitty.

[theotherjimmy]

@bogdanm Point 3 taken care of.
example: python workspace_tools/remove-device-h.py -t LPC23XX

diff --git a/hal/targets.json b/hal/targets.json
index ff77b61..a1ec3ef 100644
--- a/hal/targets.json
+++ b/hal/targets.json
@@ -281,7 +281,8 @@
         "core": "ARM7TDMI-S",
         "progen": {"target": "lpc2368"},
         "extra_labels": ["NXP", "LPC23XX"],
-        "supported_toolchains": ["ARM", "GCC_ARM", "GCC_CR"]
+        "supported_toolchains": ["ARM", "GCC_ARM", "GCC_CR"],
+        "provides": ["ANALOGIN", "ANALOGOUT", "CAN", "ERROR_PATTERN", "ETHERNET", "I2C", "I2CSLAVE", "INTERRUPTIN", "LOCALFILESYSTEM", "PORTIN", "PORTINOUT", "PORTOUT", "PWMOUT", "RTC", "SEMIHOST", "SERIAL", "SPI", "SPISLAVE", "STDIO_MESSAGES"]
     },
     "LPC2460": {
         "inherits": ["LPCTarget"],

also, I sorted the provides array.

[theotherjimmy]

@bogdanm an example of what you mean by point 2 would be helpful.

Point 4 is good, and I'm pretty sure that it may fail some builds as the following lines appear in the device.h associated with LPC23XX:

#define DEVICE_ID_LENGTH       32
#define DEVICE_MAC_OFFSET      20

which I don't handle at the moment. We could:

1. remove only the lines that end in 0 or 1, assuming that we can easily recreate them with -DDEVICE_<thing>={1|0} on the command line. Then remove the file if it only contains the header wrappers.
2. make provides a dict that replaces all #defines in the device.h file.
   Any suggestions for how we could handle this?

[bogdanm]

For point 2, see the documentation about defining target data here, especially the section macros, macros_add, macros_remove. In this case, if a target A defines provides like this:

"provides": ["p1", "p2", "p3", "p4"]

and target B inherits from A:

• if B and A provide them same thing, B doesn't need to declare provides, since it inherits it from A.
• if B also provides p5, it can do "provides_add": ["p5"]
• provides_remove also works, but I wouldn't recommend using it, that's mostly useful for macros.

To add the _add/_remove capability to provides, you just need to add "provides" to this array: https://github.com/mbedmicro/mbed/blob/master/workspace_tools/targets.py#L61

[bogdanm]

1. make provides a dict that replaces all #defines in the device.h file.

I like this. Or you can use a combined approach and do something like this (which is shorter):

"provides": ["ANALOGIN", "ANALOGOUT", "CAN", ... , {"DEVICE_ID_LENGTH": 20}, ...]

This looks a bit weird, but given the size of targets.json, it's probably more pragmatic.
And of course, even if we do this, it'd be great if we could find a way to check if this automatic conversion succeeded. Although I don't have yet any good ideas about how to do that.

[bogdanm]

Found this, not sure if it's useful or not: http://stackoverflow.com/a/12147822

[sg-]

We should reserve provides for platform defined features and macros for target/platform configuration. The base Target object should have an official list that is check against the provides implementation such that its not misused. Board specific macros should be in a file such as target.h (?). This would be a good place for the next change set which is friendly pin names :) reduction from Pinnames.h to reduce the enum copy paste that exists in inherited targets

One more thing to remove from sources:

• hal/api probably safe to just delete
• hal/common probably safe to just delete
• hal/hal needs to include objects.h
• hal/targets/... probably safe to just delete

$ grep -R . -n  -e "#include \"device.h\"" 
./hal/api/mbed_debug.h:18:#include "device.h"
./hal/api/mbed_interface.h:19:#include "device.h"
./hal/api/platform.h:21:#include "device.h"
./hal/api/semihost_api.h:19:#include "device.h"
./hal/common/assert.c:17:#include "device.h"
./hal/common/error.c:18:#include "device.h"
./hal/hal/analogin_api.h:19:#include "device.h"
./hal/hal/analogout_api.h:19:#include "device.h"
./hal/hal/can_api.h:19:#include "device.h"
./hal/hal/ethernet_api.h:19:#include "device.h"
./hal/hal/gpio_api.h:19:#include "device.h"
./hal/hal/gpio_irq_api.h:19:#include "device.h"
./hal/hal/i2c_api.h:19:#include "device.h"
./hal/hal/lp_ticker_api.h:19:#include "device.h"
./hal/hal/port_api.h:19:#include "device.h"
./hal/hal/pwmout_api.h:19:#include "device.h"
./hal/hal/rtc_api.h:19:#include "device.h"
./hal/hal/serial_api.h:19:#include "device.h"
./hal/hal/sleep_api.h:19:#include "device.h"
./hal/hal/spi_api.h:19:#include "device.h"
./hal/hal/ticker_api.h:19:#include "device.h"
./hal/targets/hal/TARGET_ARM_SSG/TARGET_IOTSS/SDK/mps2_ethernet_api.h:20:#include "device.h"
./hal/targets/hal/TARGET_ARM_SSG/TARGET_MPS2/SDK/mps2_ethernet_api.h:20:#include "device.h"
./hal/targets/hal/TARGET_Atmel/TARGET_SAM_CortexM4/i2c_api.c:17:#include "device.h"
./hal/targets/hal/TARGET_Atmel/TARGET_SAM_CortexM4/spi_api.c:17:#include "device.h"
./hal/targets/hal/TARGET_Maxim/TARGET_MAX32600/PeripheralPins.c:34:#include "device.h"
./hal/targets/hal/TARGET_Maxim/TARGET_MAX32610/PeripheralPins.c:34:#include "device.h"
./hal/targets/hal/TARGET_RENESAS/TARGET_RZ_A1H/rtc_api.c:18:#include "device.h"
./hal/targets/hal/TARGET_RENESAS/TARGET_VK_RZ_A1H/rtc_api.c:18:#include "device.h"
./hal/targets/hal/TARGET_Silicon_Labs/TARGET_EFM32/analogin_api.c:31:#include "device.h"
./hal/targets/hal/TARGET_Silicon_Labs/TARGET_EFM32/analogout_api.c:31:#include "device.h"
./hal/targets/hal/TARGET_Silicon_Labs/TARGET_EFM32/gpio_irq_api.c:31:#include "device.h"
./hal/targets/hal/TARGET_Silicon_Labs/TARGET_EFM32/i2c_api.c:31:#include "device.h"
./hal/targets/hal/TARGET_Silicon_Labs/TARGET_EFM32/lp_ticker.c:31:#include "device.h"
./hal/targets/hal/TARGET_Silicon_Labs/TARGET_EFM32/mbed_overrides.c:36:#include "device.h"
./hal/targets/hal/TARGET_Silicon_Labs/TARGET_EFM32/port_api.c:31:#include "device.h"
./hal/targets/hal/TARGET_Silicon_Labs/TARGET_EFM32/pwmout_api.c:31:#include "device.h"
./hal/targets/hal/TARGET_Silicon_Labs/TARGET_EFM32/rtc_api.c:31:#include "device.h"
./hal/targets/hal/TARGET_Silicon_Labs/TARGET_EFM32/serial_api.c:31:#include "device.h"
./hal/targets/hal/TARGET_Silicon_Labs/TARGET_EFM32/sleep.c:31:#include "device.h"
./hal/targets/hal/TARGET_Silicon_Labs/TARGET_EFM32/spi_api.c:31:#include "device.h"
./hal/targets/hal/TARGET_Silicon_Labs/TARGET_EFM32/us_ticker.c:38:#include "device.h"

[theotherjimmy]

We are looking at

[u'AACI',
 u'ANALOGIN',
 u'STDIO_MESSAGES',
 u'TSC',
 u'PORTIN',
 u'I2C_ASYNCH',
 u'DEBUG_AWARENESS',
 u'I2CSLAVE',
 u'CAN',
 u'LOCALFILESYSTEM',
 u'RTC_LSI',
 u'CLCD',
 u'LOWPOWERTIMER',
 u'RTC',
 u'ERROR_PATTERN',
 u'SPI',
 u'SERIAL_ASYNCH',
 u'SERIAL_FC',
 u'SEMIHOST',
 u'INTERRUPTIN',
 u'SPI_ASYNCH',
 u'PORTOUT',
 u'SERIAL',
 u'ANALOGOUT',
 u'SPISLAVE',
 u'PORTINOUT',
 u'PWMOUT',
 u'SLEEP',
 u'ERROR_RED',
 u'ETHERNET',
 u'I2C',
 u'SERIAL_ASYNCH_DMA']

as all current possible 'features' of a device

[theotherjimmy]

@sg- @bogdanm @0xc0170 is there anything in that list that should be removed?

[bogdanm]

@sg- @bogdanm @0xc0170 is there anything in that list that should be removed?

No idea, sorry :) But anyway, I don't necessarily think that gathering all these defines in a single place is a good idea. Something like CLCD is quite target specific, it doesn't feel like including it in Target makes a lot of snese.

[sg-]

These represent mbed features

 u'ANALOGIN',
 u'PORTIN',
 u'I2C_ASYNCH',
 u'I2CSLAVE',
 u'CAN',
 u'LOCALFILESYSTEM',
 u'LOWPOWERTIMER',
 u'RTC',
 u'SPI',
 u'SERIAL_ASYNCH',
 u'SERIAL_FC',
 u'INTERRUPTIN',
 u'SPI_ASYNCH',
 u'PORTOUT',
 u'SERIAL',
 u'ANALOGOUT',
 u'SPISLAVE',
 u'PORTINOUT',
 u'PWMOUT',
 u'SLEEP',
 u'ETHERNET',
 u'I2C',

These seem to be platform features that could be better handled?

 u'ERROR_PATTERN',
 u'STDIO_MESSAGES',
 u'DEBUG_AWARENESS',
 u'SEMIHOST',
 u'ERROR_RED',

These are extensions for specific platforms that should be in a targets.h like file

DEVICE_SERIAL_ASYNCH_DMA
CLCD
RTC_LSI

[bogdanm]

That looks like a nice classification. We could put all of them under provides, but at different levels in the target inheritance tree. Or we could choose to provide actual headers for some of them. I'm tempted to use provides for everything, since that should make adding a target a little easier, but the headers would work too.

[theotherjimmy]

We would like to use the provides array to filter test out that will never work on a platform. We need to be careful what we let into the provides arrays, as they represent hardware features that mbed provides an abstraction for. Therefore, I think that being conservative with what we let in is the better option.

[bogdanm]

We need to be careful what we let into the provides arrays, as they represent hardware features that mbed provides an abstraction for.

Only if you define it as such. If you let targets extend it with their own specific hardware, that's still a valid model, and you can still filter tests. And mbed will still provide abstractions only for the features it knows of. Maybe I don't understand the advantages of being conservative instead.

[theotherjimmy]

You're right, we don't need to worry:
http://stackoverflow.com/questions/5085392/what-is-the-value-of-an-undefined-constant-used-in-if-c

[theotherjimmy]

After a bit of grepping:

$ grep -R . -e "#.*if.*DEVICE_TSC"
$ grep -R . -e "#.*if.*DEVICE_AACI"
$ grep -R . -e "#.*if.*DEVICE_CLDC"     

I am convinced that TSC, AACI, and CLDC are all safe to remove.

[theotherjimmy]

Sorry for the CI noise. I forgot to remove a comma.

[0xc0170]

Before merging, consider this is a breaking change, as there are projects out there which are using mbed sources, but not our buildsystem, suddenly device.h is empty (yes they will update, will require to get our json file and parse it). I am not against this change, just a note that we should start announcing this type of changes in advance (aka "this changeset will be part of this release, either via issues here or mailing list"). My 5 cents

[theotherjimmy]

Thanks for your cents! This change will definitely break all build systems outside of this repo.

[screamerbg]

Build systems outside of this repo definitely have knowledge about the macros that this build system defines. E.g. to compile a target you need bunch of macros related to TARGET_ and TOOLCHAIN_ and also LIKE_CORTEX__, LIKE_MBED_, RTOS_ and so on.

[bogdanm]

Thanks @0xc0170, that's a very good point. What if we do it "less braking" then? We could automatically generate a device.h that lives outside source control, as previously suggested, which is potentially "less breaking".

[screamerbg]

@bogdanm how is that different than the upcoming config system? Shall we generate mbed_config.h from the config system instead of runtime -D macros then?

[sg-]

That still requires use of our build system so would say we should make a utility in tools but also just expect that people will use our export scripts

[bogdanm]

I think @sg- is right: if people use our export scripts, the configuration will automagically make it in there. That (people using our exporters) feels like a reasonable expectation.

[screamerbg]

@bogdanm exactly. This applies to 'features' as well

[0xc0170]

@theotherjimmy Ready for merging?

[theotherjimmy]

hold a few min. need to translate the build system patch to the other build system repo

[theotherjimmy]

Ready for merging.

[sg-]

👍

[screamerbg]

Shall we @0xc0170 ? :)