Various typos #2

johannessen · 2018-07-18T20:49:52Z

Hi cr,

I have modified pyhx870 to be a bit more useful for myself, and conceivably for others as well. Obviously I have no idea if any of this is even roughly in the direction into which you want to take this tool. Feel free to let me know what you think if you have the time.

A few brief notes:

As mentioned by email, I am not fully fluent in Python. First and foremost, therefore, it was my intention to provide some sort of Python-free interface to this tool, enabling me to read and write from and to the device using the shell command line. The two simple CLI scripts read-config.py and write-config.py accomplish that. Both are based on pyhx870/main.py.

(Am I correct in assuming that at this point, pyhx870/main.py no longer serves any purpose and could be removed?)
After cloning pyhx870 for the first time, I struggled for a while with installing the required modules, until I found out about pip and setup.py. (Yes, it’s been a looong time since I used Python before.) Other noobs might benefit from some kind of brief “installation instructions” in the readme file. I haven’t yet drafted those, as I wasn’t sure whether you’d appreciate me changing the readme.
It took some time to sort out which parts of the HX870 class dealt with low-level protocol stuff and which parts were a high-level API, so I took the liberty of separating those parts into two different classes. I guess it’d be nicer if the protocol implementation details were not inherited by the object the client uses directly, but changing that would have meant modifying the API, which I tried to avoid out of fear for breaking other clients.
Some more fields are now documented on the HTML page at https://github.com/johannessen/hx870. Since I don’t have 010 Editor, I didn’t update your template.

Cheers,
Arne

cr · 2018-07-25T12:15:02Z

Thank you so much, @johannessen, for this pull request! I still need to find time to fully review your changes.

For the future: it'll be easier to work more incrementally instead of making one huge pull request that incorporates many changes at once which will take a long time to review and discuss.

For the sake of simplicity I might land all of these changes at once and then deal with cleaning up later. I'll get back to you as soon as I can.

johannessen · 2018-07-26T19:01:45Z

Thanks a lot for your feedback! I’ll try to keep in in mind.

FWIW, it looks like GitHub would allow opening a second PR containg just the first three commits. After that one has been discussed and either merged or discarded (and the commits reverted), this PR would then just contain the newly added features.

cr · 2018-11-19T21:09:22Z

I just landed pull request #3 which was inspired by your work on the CLI. It's a little different from your design, but should plot out the direction I'd like to take the interface. Installing the module will add a hx870 command line tool that takes subcommands for executing functions on a handset. See hx870 --help for a list of currently supported subcommands. All subcommands are defined in the pyhx870.cli submodule. The new design makes it really easy to add new functionality in the form of new subcommands.

johannessen · 2018-12-02T00:00:38Z

I looked into your changes to pyhx870’s interface. I like the result a lot! (Well, except for #4…)
Much better than my approach, obviously. 👍

After merging everything, there's a few odds and ends left over here. The title of this PR doesn’t quite fit anymore. Leaving it open for now…

My config dump/flash scripts showed a crude progress bar. After all, reading and writing does take some time, so I tend to get antsy when I stare at the screen and nothing's happening. I see the new CLI doesn’t show any progress feedback yet. Was that a conscious choice on your part, or did you just not get around to implementing it?

I understand there are some Python modules that offer a “real” progress bar, but haven’t looked into those yet.

cr · 2018-12-11T14:58:44Z

Progress bars are nice, but they don't really fit with the current logging-based reporting concept. I'd suggest posting infrequent progress (every ten seconds or 10%) with log.info(). I have a progress tracking class around that we can use for that purpose. I will add it with a coming patch. Opened #5 for tracking this.

There's a quick fix for the impatient via the --debug flag that will log raw commands as they happen.

johannessen · 2018-12-23T14:59:00Z

Sounds good.

This PR currently reports progress every 3.125 % (= 1/32), which is quite generous. 1/16 should do fine, possibly even 1/8.

cr

As you might have seen, the project has evolved quite dramatically, making your PR obsolete for the most part. I have incorporated some of your changes into the latest pull-request (with more coming), so thanks a lot for that!

I would love to merge your changes to the 010 editor template. Could you verify my feedback on the enum order and make a new pull request for just the template changes? It would be great to finally get one of your PRs landed before I recklessly shift the ground under your feet.

I'm somewhat of a serially obsessive hacker, so I feel a little bad about keeping the long periods of silence. I am very thankful for your input and contributions and hope I didn't disgruntle you too much with my latest virtual disappearance from the project.

cr · 2019-04-11T07:13:37Z

convert.py

@@ -58,7 +58,7 @@
    for dev, direction, string in protocol:
        print("%s %s%s" % (dev, "> " if direction == "0" else "  < ", repr(string)[1:-1]))

-elif mode == "extract":
+elif mode == "dump":


We should leave the terminology at "dump / flash". It's a somewhat established standard.

Yeah, I figured.

The convert.py script has “usage” documentation for a mode called dump, but a part of your implementation in fact expects extract. Clearly a typo. This change fixes that in favour of dump.

(Not sure if perhaps convert.py is no longer necessary and should be removed entirely under #14.)

cr · 2019-04-11T07:14:52Z

hx870dat.bt

@@ -56,7 +56,7 @@ enum <ubyte> _MultiWatch { MW_DUAL, MW_TRIPLE };
 enum <ubyte> _ScanType { ST_MEMORY, ST_PRIORITY };
 enum <ubyte> _EmergencyLED { EL_CONTINUOUS, EL_SOS, EL_BLINK1, EL_BLINK2, EL_BLINK3 };
 enum <ubyte> _WaterHazardLED { WH_OFF, WH_ON, WH_ON_POWER_ON };
-enum <ubyte> _Lamp { L_OFF, L_3S, L_5S, L_10S, L_20S, L_30S, L_CONTINUOUS };
+enum <ubyte> _Lamp { L_OFF, L_3S, L_5S, L_10S, L_CONTINUOUS, L_20S, L_30S };


I think this was one of the places where I found an error in your data, and I double checked in my device that the enum order is indeed L_CONTINUOUS last. Can you verify this?

I know I double-checked this before making this change in your template. I’ve just checked again just to be safe, and my radio is currently set to 30 s, which is coded as 0x06.

So, in this particular case, the error does appear to be in your data. (I know I made errors in other fields, which your template helped me to correct.)

FWIW, I believe the first HX870 firmware versions didn’t offer the 20 s and 30 s settings. That’s why they’re at the end: They were added later than the “continuous” setting was. But I don’t seem to have a source to back this up, so it may just be conjecture on my part.

Thanks for checking.

pyhx870/protocol.py

cr · 2019-04-11T07:17:49Z

setup.py

-    url='https://github.com/cr/pyhx870',
-    download_url='https://github.com/cr/pyhx870/archive/latest.tar.gz',
+    url='https://github.com/cr/hx870',
+    download_url='https://github.com/cr/hx870/archive/master.tar.gz',


Good catch! I have a latest tag in all my other projects and didn't notice that this link doesn't work without. I wonder where else this is broken...

That I can’t tell you. But I’m afraid your repository is called hx870, not pyhx870, so the links are still broken…

cr · 2019-04-11T07:31:50Z

See pull request #7 and #9 for the latest changes to the code base, implementing much of your changes.

johannessen · 2019-10-23T15:03:00Z

My apologies for the very late reply. I’m afraid the sweeping changes to this project that you made earlier this year looked daunting enough to keep me from really looking into things. In the end, merging your changes into my fork turned out to be quite simple and I feel bad about not doing so much sooner.

It looks like your work addresses all of the proposals for substantive changes that were included in this pull request. That’s great!

All remaining changes in this PR just concern minor typos. While I do get your initial point about avoiding monolithic pull requests dealing with multiple issues at once, I guess at this point it would probably no longer be useful to subdivide it. What do you think?

I’ve amended the PR’s title to reflect its reduced scope.

cr

Heya, thanks for finding those nasty bugs and typos!

In its current state I can't merge due to a merge conflict with current master, probably caused by your branch being based on an older commit. The usual solution is to pull the lastest master and then git rebase your branch on master by manually resolving the merge conflicts that pop up. The process can be a bit daunting and includes a steep clearning curve if it's the first time. Would you want to give it a try (you can always --abort), or should I take this on?

cr · 2019-10-25T05:43:02Z

hx870dat.bt

@@ -56,7 +56,7 @@ enum <ubyte> _MultiWatch { MW_DUAL, MW_TRIPLE };
 enum <ubyte> _ScanType { ST_MEMORY, ST_PRIORITY };
 enum <ubyte> _EmergencyLED { EL_CONTINUOUS, EL_SOS, EL_BLINK1, EL_BLINK2, EL_BLINK3 };
 enum <ubyte> _WaterHazardLED { WH_OFF, WH_ON, WH_ON_POWER_ON };
-enum <ubyte> _Lamp { L_OFF, L_3S, L_5S, L_10S, L_20S, L_30S, L_CONTINUOUS };
+enum <ubyte> _Lamp { L_OFF, L_3S, L_5S, L_10S, L_CONTINUOUS, L_20S, L_30S };


Thanks for checking.

@cr

While porting the new fields discovered by @cr over to the HTML page at @johannessen/hx870 I spotted some minor errors in the template. By now the HTML page also contains a number of additional new fields, but I don't own 010 Editor and don't feel confident enough about its syntax to try and do more here than fix these two simple errors.

The HX870's GPS log functionality stores a history of past positions. This is completely unrelated to the radio's navigation functionality with routes and waypoints. To avoid confusion, the term "waypoints" should be avoided in the context of the GPS log. This change replaces "waypoints" with "trackpoints", which is used by GPX for this purpose. Alternatives include simply "points" or "positions".

johannessen · 2019-10-25T10:02:43Z

Sorry about that – I did a merge instead of a rebase before, and GitHub claimed there were no conflicts. Thanks for explaining how to fix this!

cr · 2019-10-25T12:44:14Z

I did a merge instead of a rebase before,

Oh, yeah, local merge commits in developer branches break established github workflows. The rebase process could become contrived and complicated if the dev branch contains merge commits. Sometimes, extracting the diff and re-applying it to a fresh branch off of master is the only viable option.

cr

LGTM

cr · 2019-10-25T13:28:31Z

Thanks for the patch!

johannessen changed the title ~~A simple CLI and some refactoring for increased usability~~ Various minor fixes/improvements (was: A simple CLI …) Dec 2, 2018

cr reviewed Apr 11, 2019

View reviewed changes

johannessen changed the title ~~Various minor fixes/improvements (was: A simple CLI …)~~ Various typos Oct 23, 2019

cr requested changes Oct 25, 2019

View reviewed changes

johannessen added 5 commits October 25, 2019 11:51

Fix GitHub URLs

ee32bbd

Fix CLI 'dump' mode parsing in convert.py

352ecfc

Fix typo in HX870 class docstring (HX890 -> HX870)

43d25a5

johannessen force-pushed the usability-update branch from 5b3309a to aa42475 Compare October 25, 2019 09:58

cr approved these changes Oct 25, 2019

View reviewed changes

cr merged commit a33a7c1 into cr:master Oct 25, 2019

johannessen deleted the usability-update branch October 25, 2019 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various typos #2

Various typos #2

johannessen commented Jul 18, 2018

cr commented Jul 25, 2018

johannessen commented Jul 26, 2018

cr commented Nov 19, 2018

johannessen commented Dec 2, 2018

cr commented Dec 11, 2018

johannessen commented Dec 23, 2018

cr left a comment •

edited

Loading

cr Apr 11, 2019

johannessen Oct 23, 2019

cr Apr 11, 2019

johannessen Oct 23, 2019

cr Oct 25, 2019

cr Apr 11, 2019

johannessen Oct 23, 2019

cr commented Apr 11, 2019

johannessen commented Oct 23, 2019

cr left a comment

cr Oct 25, 2019

johannessen commented Oct 25, 2019

cr commented Oct 25, 2019

cr left a comment

cr commented Oct 25, 2019

Various typos #2

Various typos #2

Conversation

johannessen commented Jul 18, 2018

cr commented Jul 25, 2018

johannessen commented Jul 26, 2018

cr commented Nov 19, 2018

johannessen commented Dec 2, 2018

cr commented Dec 11, 2018

johannessen commented Dec 23, 2018

cr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cr commented Apr 11, 2019

johannessen commented Oct 23, 2019

cr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannessen commented Oct 25, 2019

cr commented Oct 25, 2019

cr left a comment

Choose a reason for hiding this comment

cr commented Oct 25, 2019

cr left a comment •

edited

Loading