adafruit / Adafruit_Adalink

Python wrapper for Segger's J-Link Commander & STMicro STLink V2 to flash various ARM MCUs
MIT License
125 stars 33 forks source link

Add optional -d/--device flag #1

Closed microbuilder closed 9 years ago

microbuilder commented 9 years ago

Many MCU families (cores in this case) contain various 'members' that vary in flash and SRAM sizes, but are otherwise identical. Unfortunately, the J-Link requires a precise model number in the -device field during initialisation, meaning we would need one core file for every variant (nRF51822 XXAA, XXAC, etc.).

To solve this, an optional --device flag should be added to Adalink to provide the correct device field, overriding the default value defined by the Core class.

For example, the following command would use the default --device value in the class (currently nrf51822_xxaa):

adalink -c nRF51822 -p softdevice.hex bootloader.hex app.hex

If we want to indicate we are using 32KB SRAM parts, though (nrf51822_xxac) we could enter the following command, overriding the default:

adalink -c nRF51822 -d nrf51822_xxac -p softdevice.hex bootloader.hex app.hex
microbuilder commented 9 years ago

Perhaps we should also have an option in the base class to try to detect the device version via register reads, and providing None as an argument will run the auto-detect procedure?

I added an LPC824 Core as a quick test since I had a board handy, for example. The .info command reads the DEVICE_ID register and based on this we can determine the device type for the J-Link constructor (See https://github.com/adafruit/Adafruit_Adalink/blob/master/adalink/cores/lpc824.py#L12).

We'd still have to provide something to the constructor before we can read these values, though, but in most cases the default 'device' type should work for auto-detect purposes.

microbuilder commented 9 years ago

Or perhaps a function just to validate the supplied device number (including the default), to make sure the device we are connecting to is actually the one we specified? Almost every MCU has a register you can read to get the device version, etc.

So if I specify 'nrf51822_xxac' (32KB SRAM) as the device type, we run the Verify Device function automatically in the constructor or when connecting, and if it detects this is actually an 'nrf51822_xxaa' (16KB SRAM) we return an error?

Perhaps I'm getting too complicated here, but I'm just thinking how we can avoid flashing firmware that doesn't fit on the target device, etc.

microbuilder commented 9 years ago

I'm not sure if this is the ideal way to do it, but as an experiment I added a new detect_segger_device_id function to the Core base class that tries to automatically determine the right string based on some HW registers.

I'd suggest we use this at startup to verify that the -d/--device string being used is actually the right one, and I return this detected field in the --info command:

$ adalink -c nRF51822 -i
adalink version: 1.0.1
Hardware ID : QFACA10 (32KB)
Segger ID   : nRF51822_xxAC
SD Version  : S110 8.0.0
Device Addr : C6:E7:A8:18:E5:94
Device ID   : F74B089036D740D2

For example, see: https://github.com/adafruit/Adafruit_Adalink/blob/master/adalink/cores/lpc824.py#L80

microbuilder commented 9 years ago

Adding a sub-parser based on the selected core might make more sense here since argparse support this, and then we have core specific arguments, which might save us some headache moving forward. The -d/--device field can be handled there, and then we will also get a list of valid options in the help menu for each core.

tdicola commented 9 years ago

I was looking into subparsers and it looks like it's relatively straight forward to give each core an optional subparser. However one thing that's required with subparsers is that the subparser is specified with a positional argument instead of a flag-based argument. So right now we have a -c/--core flag-based argument to pick the core, like:

adalink --core nrf51822 --program foo.hex --wipe

If we switch to a subparser based on the core name then the syntax would change to:

adalink nrf51822 --program foo.hex --wipe <optional nrf51822-specific subcommands, etc.>

I don't think this is too bad to do, what do you think though? Any concerns switching the core type from a flag parameter to a positional parameter that specifies the subparser?

microbuilder commented 9 years ago

That seems OK to me.

microbuilder commented 9 years ago

Solved with the latest commit making the core a subparser.