Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for Black Magic Probe #186

Closed
wants to merge 9 commits into from
Closed

Add support for Black Magic Probe #186

wants to merge 9 commits into from

Conversation

micooke
Copy link
Contributor

@micooke micooke commented Aug 21, 2017

In support of issue #183 - Add Black Magic Probe (BMP) upload method
Adds Black Magic Probe as a programmer option.
This is basically a merge of work done by @rogerclarkmelbourne in arduino-nRF5-customised, with the addition of bootloader uploading as per @sandeepmistry openocd reference in platform.txt

@rogerclarkmelbourne
Copy link
Contributor

I could be wrong, but last time I asked, Sandeep said he would not include this unless it was via OpenOCD

Hence why I just made a customised version of the repo for me and a few other people who wanted BMP support.

However, I don't pull from Sandeeps repo that often, so my version is likely to get out of date quickly

@micooke
Copy link
Contributor Author

micooke commented Aug 21, 2017

@sandeepmistry - i apologise, looks like i merge from v0.3.0 so that's incorrect in platform.txt

Also i made some changes which you made anyway from 0.3.0 to 0.4.0. The difference being i use protocol instead of e.g. program.protocol.

As per rogers comment, BMP uses the gdb you distribute with this repo. If your happy to support this, ill resubmit this without the changes to the existing programmers (and fix the version reference)

@dlabun
Copy link
Collaborator

dlabun commented Aug 21, 2017

Right now the rep uses OpenOCD for all of the programming activities... Can the BMP work with OpenOCD?

@micooke
Copy link
Contributor Author

micooke commented Aug 21, 2017

The repo uses openOCD for all uploading, BMP uses arm-none-eabi-gdb (inside {runtime.tools.gcc-arm-none-eabi-5_2-2015q4.path}/bin/). This is included by this repo, the gcc inside is used for the compilation.

I haven't investigated whether BMP can use openOCD because of the fact that it runs its own gdb server which IMO is much more powerful. It also hosts a uart port (both gdb server and uart off 1 usb) so its a very nifty device.

@micooke
Copy link
Contributor Author

micooke commented Aug 21, 2017

Once again, if you not happy with the concept then please dont accept this - ill resubmit a pull request with only the BMP addition.

If you dont accept the concept i obviously wont bother sending the updated pull request :)

@rogerclarkmelbourne
Copy link
Contributor

You would need to ask Gareth of Blacksphere whether the BMP definitely didnt work with OpenOCD, but from their website https://1bitsquared.com/products/black-magic-probe

It says

Black Magic Probe gets rid of intermediate programs like OpenOCD or STLink server.

Which implies that it does the same job as OpenOCD, hence I can't see how it could work with OpenOCD in its current form

@micooke
Copy link
Contributor Author

micooke commented Aug 21, 2017

@rogerclarkmelbourne - thanks for that!
@sandeepmistry, @dlabun - i reverted the changes i made to the openocd programmers, so this should be ready to merge - if thats what you want to resolve #183.
(I didnt know i could just commit the update, i thought i would have to start a new pull request)

@rogerclarkmelbourne
Copy link
Contributor

rogerclarkmelbourne commented Aug 22, 2017

Note. The problem with the version in my repo is that you need to select the serial port of the BMP GDB device prior to uploading, as this is not the same serial port which carries the "Serial.print()" data

On windows I now have a script which automatically finds the COM port for the BMP GDB server, but I've not had chance to update my repo.
The same could probably be done on Linux and on OSX, but as my main dev is on Windows, I don't have the time or inclination to port that code to other platforms

It should also be noted that to use the BMP on Windows, you need to load a driver.

So this PR on its own without the driver files is not going to work for Windows users

@micooke
Copy link
Contributor Author

micooke commented Aug 22, 2017

This is true, but the other programmers require driver installation as well. And on Windows 10 the BMP works without requiring (or being able to use) the BMP drivers.

So i guess i would need to add a link in the readme? @rogerclarkmelbourne - these are hosted in blackspheres github now, correct? I used whats in your repo.

Regarding com port selection - this is required for all the avr boards so i dont see that as an inconvenience. However i would be interested if you could upload your autoselect tool/script (not for inclusion in this). As a side note, how did you end up naming the com ports in windows 10?

@rogerclarkmelbourne
Copy link
Contributor

On AVR, you select a port and its the same for both Serial.print and also for uploading

With the BMP it has 2 USB serial devices. So, if you were looking at the output of Serial.print with COM5 selected, you'd need to change the com port selection to COM4 before you uploaded, otherwise it won't work

@micooke
Copy link
Contributor Author

micooke commented Aug 22, 2017

This is true, but you only have to open up the serial monitor once as it is not reset when you perform a new upload. So you can open the serial monitor on the UART port, select the GDB port then upload to your hearts content without touching the serial monitor. Or you can open the UART in a separate terminal, or you can connect to UART over BLE with the appropriate code, or you may not be using UART it at all.

@dlabun
Copy link
Collaborator

dlabun commented Aug 22, 2017

Sandeep is going to be the one that makes the final call on this one... If it was OpenOCD compatible I'd feel comfortable merging the pull request.

@dmikhalsky
Copy link

One small correction: you don't need to detach and reattach after "mon erase_mass", you can "load" immediately after

@micooke
Copy link
Contributor Author

micooke commented Aug 22, 2017

Thanks for the correction! I know i added that in for some reason, so ill just have to check i dont get any issues by taking it out.

Thanks 👍

Copy link
Owner

@sandeepmistry sandeepmistry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@micooke thanks for submitting this!

Have you had a chance to try @dmikhalsky's comment from #186 (comment) ?

platform.txt Outdated
@@ -129,3 +129,26 @@ tools.openocd.erase.pattern=
tools.openocd.bootloader.params.verbose=-d2
tools.openocd.bootloader.params.quiet=-d0
tools.openocd.bootloader.pattern="{path}/{cmd}" {bootloader.verbose} -f interface/{program.protocol}.cfg -c "{program.setup_command}" -f target/{upload.target}.cfg -c "init; halt; nrf51 mass_erase; program {{{runtime.platform.path}/cores/nRF5/SDK/components/softdevice/{softdevice}/hex/{softdevice}_{upload.target}_{softdeviceversion}_softdevice.hex}} verify reset; shutdown;"

# blackmagic probe upload
tools.bmp_upload.cmd=arm-none-eabi-gdb
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more descriptive name than bmp_upload would be nice, maybe blackmagicprobe?

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandeepmistry sounds good

platform.txt Outdated

# blackmagic probe upload
tools.bmp_upload.cmd=arm-none-eabi-gdb
tools.bmp_upload.path={runtime.tools.gcc-arm-none-eabi-5_2-2015q4.path}/bin/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use {compiler.path} here or another key, just to make it easier to upgrade the toolchain in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a direct copy from the openocd path in boards.txt. Ill investigate (because im not familiar enough with this work) and make changes if there is no issue

Copy link
Contributor Author

@micooke micooke Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandeepmistry - doesnt look like we have access to this as a global parameter :
Arduino-IDE-1.5-3rd-party-Hardware-specification.

It might be better to add a runtime define path to the gcc tools and openocd around line 20, like they do in
esp8266 platform.txt

After some experimenting, tools only have access to their recipe parameters and the default global parameters. I looked at the platform link, the tool uses a static link not the global (because it doesnt work).

Compiler options can use any global you define, which doesnt help.

Some ways to make it easier to upgrade:

  1. Remove the version number from the folder - maybe create a changes.txt or readme in the base folder for each tool?
  2. Keep it how it is - i noticed that samd atleast uses static version numbers
  3. Define the tool..path (only) at the top, and the rest of the parameters in a block. This is pretty messy though

platform.txt Outdated
tools.bmp_upload.cmd=arm-none-eabi-gdb
tools.bmp_upload.path={runtime.tools.gcc-arm-none-eabi-5_2-2015q4.path}/bin/

tools.bmp_upload.upload.speed=230400
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do different boards need different values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that ive noticed. Ive tried two different nrf51822 chips over 3 boards and have never had an issue.

@rogerclarkmelbourne - have you had any problems with this upload speed with your nRF51 or nRF52 boards?

@micooke
Copy link
Contributor Author

micooke commented Sep 5, 2017

Ill action these changes today (hopefully)

@micooke
Copy link
Contributor Author

micooke commented Sep 5, 2017

All sorted. @dmikhalsky - for reference, i reverted back to the version that was giving me issues that resulted in the detach/attach. It solved an issue that i just realised was due to an incorrect upload command ( -ex quit" without a leading "). I later corrected this, but never linked the cause and effect.

@micooke
Copy link
Contributor Author

micooke commented Sep 8, 2017

Please don't merge this yet!

With my variants project I found that the tool referencing in programmers.txt is incorrect for bmp and openocd (only if called externally). In short it needs a preceeding sandeepmistry: . The tool blackmagicprobe wasnt found and i suspect it was using another openocd.

Same goes for the softcore (once again only when referenced externally).

This needs a bit more of a look in to.

@micooke
Copy link
Contributor Author

micooke commented Sep 12, 2017

@sandeepmistry - this should be ready to merge.

I added defaults for upload.tool, bootloader.tool for jlink, stlink and cmsisdap so these references shouldnt be needed anymore in boards.txt.

I needed to make a modification with my smartwatch variants project, the easiest/only way was to duplicate the folder structure to store the softdevices as there is no global parameter to reference an external cores base directory.

* Add empty softdevice version for 'none'
* Allow external variants to use either blackmagicprobe or openocd as the bootloader
* .gitignore so it doesnt ignore hex files inside softdevice/none/
* remove cd to {build.path} in blackmagicprobe erase routine
@micooke
Copy link
Contributor Author

micooke commented Sep 13, 2017

Fixed.
Arduino syntax is kinda confusing, tell me im not going insane?
syntax example
stlink.program.tool=openocd
stlink.bootloader.tool=sandeepmistry:openocd

I broke this in a previous update because i e.g. had them both as sandeepmistry:blackmagicprobe

  • If bootloader.tool is blackmagicprobe i get the error cannot find 'blackmagicprobe' if i use an external variant (all good inside this repo though). Note : i didnt use openocd in this example as that is in the arduino core, so the arduino version gets found.
  • If program.tool is sandeepmistry:blackmagicprobe i get the error missing 'program.params.verbose' configuration parameter. yay for consistency 👍

I also updated .gitignore so that it only ignores .hex, .txt inside s* softdevices (i.e. not none)

@dmikhalsky
Copy link

Mike, what do you mean by "external variant"?

@micooke
Copy link
Contributor Author

micooke commented Sep 13, 2017

(its Mark ☺️) - im talking about a variant that does not reside in the same directory as this core, but has the same architecture (nRF5) and references this core (also called nRF5).

In this example im talking about my smartwatch repo https://github.com/micooke/arduino-nRF5-smartwatches

@dmikhalsky
Copy link

That's what I thought, thank you, Mark!

@dlabun
Copy link
Collaborator

dlabun commented Sep 13, 2017

I have to ask, why do we need to change how all of the other programmers work? Can't we just add the BMP without touching what we already know works?

@micooke
Copy link
Contributor Author

micooke commented Sep 13, 2017

You don't as it doesnt affect any of the variants in this repo, but it means that i won't be able to use openocd for the smartwatch variants in my repo unless i have openocd installed elsewhere.

From memory the sam and samd arduino board packages have it?

In terms of programmers i have jlink, stlink v2 and bmp.

Sorry to hijack this PR with essentially a bugfix/feature request, i just thought it was the best spot. Im happy to pull out the changes and spawn another PR to address this separately.

@dlabun
Copy link
Collaborator

dlabun commented Sep 14, 2017

I'm concerned that this PR went from adding support for another programmer to making a lot of underlying changes to how the core works... Instead of just testing the BMP we now have to test every programmer.

@micooke
Copy link
Contributor Author

micooke commented Sep 14, 2017

For the existing programmers its just providing explicit linkage to the programmer details. Its easy enough to test, just try and burn a bootloader with one of the openocd programmers. I took out the other additions i made (adding upload.tool etc - i realised this detracted from the PR)

But tell you what, tomorrow ill pull out those changes and just have the bmp additon.

@sandeepmistry mentioned transitioning to the core openocd - once that happens my recommended sandeepmistry:openocd changes become irrelevant. I just had it in there for consistency because using bmp to burn a bootloader didn't work with my smartwatch repo without the sandeepmistry: prefix. It does work with the openocd options because it finds the arduino core and uses that.

Sandeep has manually merged the other changes regarding softdevice into the core, my fork is a little outdated so they come up as additions. Any advice on how to fix this is welcome, but they will probably be ignored in the merge.

@micooke
Copy link
Contributor Author

micooke commented Sep 14, 2017

Honestly, im easy on what you want to do with this but adding bmp without the prefix does not personally help me - but it will still be a benefit to people wanting to use bmp with your variants.

Its your repo - totally up to you.

Also, i wanted to say sorry for all the incremental updates. I made an update, thought it would be instant and didnt notice it broke until i restarted Arduino. Then one i got something working i rolled it out to every example, which didnt work because program.tool and bootloader.tool have different syntax (im guessing bootloader is the exception and that they are all supposed to work like program.tool). Anyway, trial and error.

Purge non-blackmagicprobe changes
* remove bootloader.tool for all openocd methods
* remove s prefix to softcore hex and txt search in .gitignore
@dlabun
Copy link
Collaborator

dlabun commented Sep 18, 2017

Let's take it one step at a time... First let's get BMP working with the core and once we're sure everything is still working correctly we can attack the prefix issue.

@micooke
Copy link
Contributor Author

micooke commented Sep 18, 2017

It should be ready to test. Unless you want me to remove the prefix from bmp?

I have tested it with and without the prefix with the waveshare variant and it works either way.

@dmikhalsky
Copy link

Will the commit be merged?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants