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

Fix system update check #67

Closed
wants to merge 1 commit into from
Closed

Conversation

ski7777
Copy link
Member

@ski7777 ski7777 commented Dec 25, 2016

@rkunze I think this should work better.

@rkunze
Copy link
Member

rkunze commented Dec 25, 2016

No. The update script is meant to install exactly the specified official version, regardless of the current version. The do-nothing shortcut only applies if this exact version is installed already.

A check (and probably a warning) if you are about to downgrade from a newer to an older version belongs into the system update GUI app.

@rkunze rkunze closed this Dec 25, 2016
@ski7777
Copy link
Member Author

ski7777 commented Dec 25, 2016

For the case of using the script with my currently in devolpment update app, there will be no problem. But you should check the following up:
Here we generate the version file:

GIT_VERSION=$(git describe --tags --match='v*' 2>/dev/null)
if [ -n "$GIT_VERSION" ] ; then
  BASE_VERSION=$(cat board/fischertechnik/TXT/rootfs/etc/fw-ver.txt)
  if [ "v$BASE_VERSION" = "${GIT_VERSION%%-*}" ] ; then
    echo ${GIT_VERSION#v*} > $TARGET/etc/fw-ver.txt
  else
    echo "Version number $GIT_VERSION from 'git describe' does not match the base version $BASE_VERSION"
    echo "Please fix the base version in board/fischertechnik/TXT/rootfs/etc/fw-ver.txt"
    exit 1
  fi
fi

So my current build says:

$ cat /etc/fw-ver.txt
0.9.2-67-g40cd54a

So running

#!/bin/sh
...
TARGET_VERSION="$1" #set to 0.9.2
CURRENT_VERSION="$(cat /etc/fw-ver.txt)" #set to 0.9.2-67-g40cd54a
...
if [ "${CURRENT_VERSION}" = "${TARGET_VERSION}" ]; then
  echo "Already running ${CURRENT_VERSION}"
  exit 3
fi
...

will not exit with code 3. It will update.

Rapahel

@rkunze
Copy link
Member

rkunze commented Dec 25, 2016

Yes, that is exactly as desired. Version 0.9.2-67-g40cd54a is not version 0.9.2, so version 0.9.2 will be installed.

The script does not care if you upgrade or downgrade. It just installs whatever version is passed on the command line.

@ski7777
Copy link
Member Author

ski7777 commented Dec 25, 2016

We could drop these lines because there won't be 0.9.2 on a TXT with normal build.

Raphael

@rkunze
Copy link
Member

rkunze commented Dec 25, 2016

But there will be a 0.9.3, and later versions. And checking if you are about to installl exactly the same version you are running right now and aborting if you do makes sense - no need to waste time and bandwidth to change absolutely nothing.

It doesn't work with 0.9.2 because the image files are actually built from two commits later than v0.9.2 - that was an error on my part while building the release files that won't happen again on later releases (I hope). But it doesn't change the fact that the version check in system-update as it is now does exactly what it is supposed to do.

@harbaum
Copy link
Member

harbaum commented Dec 27, 2016

Talking about firmware version checks: I don't understand your version flag changes in the manifests and the fact that most of them are marked not to work with the 0.9.3 release anymore. Can you please explain this? What changes will break app compatibility? And why?

@rkunze
Copy link
Member

rkunze commented Dec 27, 2016

See ftCommunity/ftcommunity-apps@5e1c24e#commitcomment-20303277

The main problem is that there is no more 'invisible' TxtControlMain background process in v0.9.3 (because ftrobopy supports direct/auto mode now), but all apps on the store still try to connect to TxtControlMain on localhost:65000. And unconditionally fixing this would break these apps for v0.9.2 because the ftrobopy in v0.9.2 still has no 'auto' mode.

@harbaum
Copy link
Member

harbaum commented Dec 27, 2016

I am obviously late for the discussion. But aren't there trivial fixes for this? E.g. if ftrobopy knows it's running on TXT (or at least arm cpu) and doesn't find port 65000 open then it may simply fall back to direct mode or the like?

@rkunze
Copy link
Member

rkunze commented Dec 27, 2016

Yes, for this particular problem this would be a trivial (if a bit hacky) fix.

But a general solution that allows us to support multiple app variants for different firmware versions by simply creating a branch is better, IMO.

@harbaum
Copy link
Member

harbaum commented Dec 27, 2016

It's by no means "hacky" to use "localhost" as an indicator that some special local access method is possible. Imho it's actually pretty elegant as it makes live for the app developer pretty easy. The app developer just tells ftrobopy what it's supposed to connect to. And it's up to ftrobpopy to decide how it's doing that.

The current solution requires the app author to explicitly distinguish between remote and local usage.

It's ok if you won't do it that way. Bu tthen i'll not start to maintain several versions of my apps. Instead i'll make sure that my apps cope with the old and the new api by trying to autodetect what's going on. I'd prefer not to have such a hack in every single app. But if that's the only way to e able to support all ftrobopby version then i'll just do that.

@rkunze
Copy link
Member

rkunze commented Dec 27, 2016

Ftrobopy already has an 'auto' mode for automatically figuring out how to talk to the TXT. Making 'localhost' duplicate this instead of connecting to localhost as requested is hacky.

@harbaum
Copy link
Member

harbaum commented Dec 27, 2016

"auto" is actually the bad name as in this particular case we want to distinguish between "remote" and "local". So just remove the "auto" mode and call it "localhost" as this is exactly what the "auto" setting is supposed to mean: "i am on the same device, please let me talk directly to the hardware" . Very nice, very clean ....

@rkunze
Copy link
Member

rkunze commented Dec 27, 2016

Instead i'll make sure that my apps cope with the old and the new api by trying to autodetect what's going on

If you want to do this, the best way is probably to check ftrobopy.__version__ and decide on that basis wheter to use auto mode (available in version 1.56 and up) or not.

@harbaum
Copy link
Member

harbaum commented Dec 27, 2016

So now i have to distinguish between three modes: 1) remote, 2) localhost on old version and 3) auto on new version. Sounds cumbersome to me but as you already enforce that I have no other choice than to follow.

I don't like that but i can live with that. But please give me the time to update all of my apps _before_you release 0.9.3

@harbaum
Copy link
Member

harbaum commented Dec 27, 2016

I have started to remove apps that I don't think are needed anymore and which will break with the next update. Imho we should try to reduce the number of apps to a total minimum to keep the efforts for app developers low.

@rkunze
Copy link
Member

rkunze commented Dec 27, 2016

"auto" is actually the bad name as in this particular case we want to distinguish between "remote" and "local"

No. "auto" mode means "figure out where you are running and try to connect however you can" and actually tries to connect via the "normal" TXT ip adresses (192.168.7.2, 192.168.8.2, 192.168.9.2) if it detects that it does not run on a TXT.

@harbaum
Copy link
Member

harbaum commented Dec 27, 2016

Ah, ok.Anyway, I'l lkeep the cube app and brickly as they are the only I apps i still actively use. That's easy to maintain those two. And the rest may be re-added later if i find the time to maintain them.

@rkunze
Copy link
Member

rkunze commented Dec 27, 2016

So now i have to distinguish between three modes: 1) remote, 2) localhost on old version and 3) auto on new version.

No. You just need to distinguish between ftrobopy versions before 1.56 (without auto mode) and later. I'd do it like this (original example swiped from Brickly and stripped down):

import ftrobopy
txt_ip = os.environ.get('TXT_IP')
txt = None
if txt_ip:
    try:
        txt = ftrobopy.ftrobopy(txt_ip, 65000)
    except:
        print("TXT init failed", file=sys.stderr)
else:
    # should use a real version compare here, but take it as pseudo code
    if ftrobopy.__version__ >= "1.56":
        try:
            txt = ftrobopy.ftrobopy('auto')
        except:
            print("TXT init failed", file=sys.stderr)
    else:
        print("There is no TXT", file=sys.stderr)

And if you ditch support for ftrobopy < 1.56 and it's OK to use the address from TXT_IP as fallback only (i.e. have ftrobopy prefer direct mode, localhost or one of the other standard addresses, in that order), you can reduce it further to simply

import ftrobopy
txt = None
try:
    txt = ftrobopy.ftrobopy('auto', special_connection=os.environ.get('TXT_IP'))
except:
    print("TXT init failed", file=sys.stderr)

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

Successfully merging this pull request may close these issues.

None yet

3 participants