Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

dist/tools: update serial boot loader script #5784

Merged
merged 2 commits into from
Aug 31, 2016

Conversation

PeterKietzmann
Copy link
Member

Updates the serial boot loader script for TI CC13xx/CC2538/CC26xx to version 2.1. Took code from here: https://github.com/JelmerT/cc2538-bsl

Hopefully replaces #5760. @cgundogan please test!

@PeterKietzmann PeterKietzmann added the Area: tools Area: Supplementary tools label Aug 30, 2016
@PeterKietzmann
Copy link
Member Author

@gebart why did you add the "in progress" label? And how??? Never seen it before.

@jnohlgard
Copy link
Member

I don't understand. I haven't seen this PR until now. Is there some github hook setting something in my name?

@cgundogan
Copy link
Member

fails for python3, please add the python version explicitly at the top of the script

@cgundogan
Copy link
Member

please address our offline discussion (remove python from the command and make file executable instead, otherwise python will still use the system default and thus ignore env python2)

@cgundogan
Copy link
Member

Works like a charm! Please squash.

@cgundogan cgundogan added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 30, 2016
@PeterKietzmann PeterKietzmann force-pushed the tools_update_cc2538-bsl branch from 32dcb0a to 5eafb07 Compare August 30, 2016 21:41
@PeterKietzmann PeterKietzmann added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 30, 2016
@cgundogan
Copy link
Member

Murdock is green. ACK&GO

@cgundogan cgundogan merged commit 924f275 into RIOT-OS:master Aug 31, 2016
@PeterKietzmann PeterKietzmann deleted the tools_update_cc2538-bsl branch September 2, 2016 14:45
@kYc0o
Copy link
Contributor

kYc0o commented Oct 4, 2016

Uhmmm... just realised that this break support on my mac...

@kYc0o
Copy link
Contributor

kYc0o commented Oct 4, 2016

Can someone with OSX can confirm? @emmanuelsearch @thomaseichinger @Yonezawa-T2 ? Just try BOARD=cc2538dk make flash on any example, hello-world or default, the output would look like env: python2: No such file or directory.

@thomaseichinger
Copy link
Member

@kYc0o I can confirm this. Python 2.x is still OS X standard, only custom Python 3.x installations have a python3 binary.

@kYc0o
Copy link
Contributor

kYc0o commented Oct 4, 2016

Why it is python2 forced? On my OS X I have several binaries:

python
python2.6
python2.7
python3
python3.5
python3.5m

And the linked version to the python executable is 2.7.10.

Maybe if we want to enforce a Python version we should explicitly indicate which one?

edit: just for the record, with only python on the script it works again.

@thomaseichinger
Copy link
Member

AFAIK it is a Linux naming convention to clearly separate Python 2.x and Python 3.x branch binaries.

@OlegHahm
Copy link
Member

OlegHahm commented Oct 4, 2016

How do other Python 2 scripts handle this on OSX?

@kYc0o
Copy link
Contributor

kYc0o commented Oct 4, 2016

Before this change only python was executed, and so in the other afaik. And it works.

@OlegHahm
Copy link
Member

OlegHahm commented Oct 4, 2016

Yes, but this works only on systems where Python 2 is the default or for scripts that are compatible to both Python versions. There are many more scripts (outside RIOT) that only support Python 2 and I wonder how they deal with the special situation on OSX.

@kYc0o
Copy link
Contributor

kYc0o commented Oct 4, 2016

Oh well I don't have experience on other python scripts... now my questions is: Should we (OS X users) create a symlink to a python2 from our current default python? Or consider that flashing cc2538 based boards in RIOT is broken? Maybe we can try some more specific executable like python2.6? would this work in Linux? (so many questions actually...)

@thomaseichinger
Copy link
Member

According to PEP394 python2 and python3 are the correct semantics. Since this is rather new (accepted in 2012) I'd say OS X users are left to themselves when maintaining a workable development environment outside of Xcode. As usual.

@OlegHahm
Copy link
Member

OlegHahm commented Oct 4, 2016

@kYc0o, sure python2.6 would work on Linux - if Python 2.6 is installed. Hence, this is not viable solution. We could either make the script Python 3 compatible, transform it into a shell script that would determine the correct Python executable or (probably the preferred) solution leave a note for OSX users to create the symlink.

@OlegHahm
Copy link
Member

OlegHahm commented Oct 4, 2016

@thomaseichinger, "rather new" and 2012? Are you kidding me? Apple seems to change their power supply adapters every week, but do not manage to update their software within four years?

@thomaseichinger
Copy link
Member

@OlegHahm I forgot how hard it is to communicate sarcasm in written form. ;)

@OlegHahm
Copy link
Member

OlegHahm commented Oct 4, 2016

No worries, message was received correctly. :) However, Apple still manages to surprise me.

@aabadie
Copy link
Contributor

aabadie commented Oct 5, 2016

I just saw the messages here and had a look at the changes. To me, we should make it python 3 compatible, I think it's a good practice.
What was the error on python 3 ? Maybe I can help.

@cgundogan
Copy link
Member

@aabadie the script was adopted from [1]. I guess the problem is that if we are going to adapt it to python3, then we need to invest more time in maintaining that file (in case of upstream updates). You could, however, try to adapt the script to python3 in the usptream repository (:

[1] https://github.com/JelmerT/cc2538-bsl

@aabadie
Copy link
Contributor

aabadie commented Oct 5, 2016

@cgundogan, yeah I agree, I had the same idea in mind but in the mean time, I opened #5909, if you want to have a look.
Someone can contribute it upstream if needed.

@kYc0o
Copy link
Contributor

kYc0o commented Oct 5, 2016

If we want to strictly follow upstream, shouldn't it be a submodule? Then it can be updated automatically from such upstream repo (thus our possible contributions to upstream would be reflected here).

@aabadie
Copy link
Contributor

aabadie commented Oct 5, 2016

PR opened upstream. That would be great if someone could test it (I don't have a cc2538).

@miri64
Copy link
Member

miri64 commented Oct 5, 2016

Curious that their CI system is already configured to check python3...

@miri64
Copy link
Member

miri64 commented Oct 5, 2016

@aabadie
Copy link
Contributor

aabadie commented Oct 5, 2016

Curious that their CI system is already configured to check python3...

Indeed and also python 2.7. In fact, I changed a string formatting somewhere that fails with python 2.6, the other changes are only cosmetic (pep8).
I don't understand why @cgundogan add a failure with python3...

@JelmerT
Copy link
Contributor

JelmerT commented Oct 5, 2016

Thanks everyone for the updates on the script and the upstream PR. I will go over it and get it merged after testing with some HW. We wanted to make sure it was 2 and 3 compatible (I gave up on 2.6 at some point), so if there are problems with python3, please open an issue or PR.

@kYc0o Embedding the script here as a submodule was also my idea and I tried this before: #4237 but it never got approved, feel free to re-open.

@kYc0o
Copy link
Contributor

kYc0o commented Oct 6, 2016

Oh yeah I didn't know. Yeah it's true that submodules here are not really on the convention, and adding it as a package seems too much for a single file.

Given that the script should work with all (rather new and used) versions of python, we maybe might consider to change it (again) for the use of the python binary?

edit: sorry this last thing is already done on #5909

@miri64 miri64 added this to the Release 2016.10 milestone Oct 14, 2016
@JelmerT JelmerT mentioned this pull request Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants