semuconsulting / pyubx2

Python library for parsing and generating UBX GPS/GNSS protocol messages.
BSD 3-Clause "New" or "Revised" License
165 stars 65 forks source link

Added UDP UBX messages #119

Closed tridge closed 1 year ago

tridge commented 1 year ago

This is part of an effort to implement u-blox firmware update. I have a working fw update for F9P here: https://github.com/tridge/junkcode/blob/master/u-blox-fw-update/ubx_update.py Example usage:

ubx_update.py --port /dev/ttyUSB1 new_fw.bin --baud 38400 --flash-baud=460800 --verbose=2

still needs a fair bit of work. I plan on supporting updates over MAVLink and DroneCAN as well for ArduPilot, just starting with the easiest one on serial.

semuadmin commented 1 year ago

Hi @tridge

Many thanks for this - much appreciate you taking the time to share it.

Few quick questions: 1) How many different u-blox devices have you successfully tested this on? Main concern about enabling firmware update functionality is the risk of bricking the device if it goes wrong. All the currently supported UBX commands are benign in that their effects can always be reversed. 2) What's your documentation source for the UPD messages (I don't see them in the standard F9P interface spec)? I need to ensure it's in the public domain if I'm to release in a public repo under BSD-2. 3) Just to save me some time on the review, what's the reason for the change to ubxhelpers.py (for i in range(min(atts,len(valb))):)?

... and a couple of requests:

4) Please use the supplied PR template. 5) Please ensure all your commits are signed.

Cheers.

semuadmin commented 1 year ago

Hi @tridge

Just noticed you've added two new commits. Let's try to keep this PR about the UPD firmware messages please, and drop the other changes. You can always submit separate PRs for these.

Better still, outline your propsed changes in the Discussion channel BEFORE submitted any PRs - might save us both some time.

Thanks again.

tridge commented 1 year ago

are you available to discuss the approach sometime? I sent an email to your SEMU contact address

semuadmin commented 1 year ago

are you available to discuss the approach sometime? I sent an email to your SEMU contact address

I don't normally respond to direct email. This is a public repo so I prefer to keep all discussions in the public domain, as they may help other contributors. The Discussion Channel is the best place if you have any queries - we'll do our best to get back you within 24 hours on working days.

I would also refer you to the contributors' guidelines in CONTRIBUTING.md.

Hope this helps.

tridge commented 1 year ago

@semuadmin no problem, discussion started in #120

semuadmin commented 1 year ago

@tridge

I haven't had a chance to review the changes in detail, but I can see one fairly obvious howler in ubxreader.py which is causing 35 of the 210 automated unit test cases to fail:

self._timeout = float(kwargs.get("timeout", None))

... will obviously cause a TypeError exception if the "timeout" keyword argument is omitted (as it will be in the vast majority of use cases). Also, does the timeout value really need to be a float? Suggest instead:

self._timeout = int(kwargs.get("timeout", 0))

and updating the if statement in _read_bytes() to

if self._timeout != 0: (or even just if self._timeout:)

semuadmin commented 1 year ago

@tridge

Been looking at the changes and am wondering whether the root issue here isn't simply that the UPD-WRITE message should be defined as variable length rather than fixed length, i.e. rather than:

    "UPD-WRITE": {
        "address": I4,
        "size": I4,
        "data": "A512",
    },

make it:

    "UPD-WRITE": {
        "address": I4,
        "size": I4,
        "groupdata": ( # repeating group * size
            "size",
            {
                "data": X1,
            },
        ),
    },

If i've misinterpreted the meaning of the size attribute and it doesn't represent the number of data bytes, then define the message as what u-blox call a 'variable by length' message - pyubx2 will work out the length automatically, i.e.

    "UPD-WRITE": {
        "address": I4,
        "size": I4,
        "groupdata": (
            "None",
            {
                "data": X1,
            },
        ),
    },

Can't say for sure as I wasn't party to the protocol deconstruction, but this seems plausible to me.

tridge commented 1 year ago

Can't say for sure as I wasn't party to the protocol deconstruction, but this seems plausible to me.

thanks! I'll give it a try. You are probably right

semuadmin commented 1 year ago

@tridge My apologies, but I've had direct feedback from u-blox to the effect that including private UPD* message definitions in an open-source repo would violate the terms of use. I appreciate your angle on this, but I've decided not to accept the PR at this point.