ncssar / radiolog

SAR radio log program
Other
15 stars 3 forks source link

Clean up install.md #467

Open erinn opened 2 years ago

erinn commented 2 years ago

As a new user coming in it appears the install.md file may need some love. Is GISinternals still needed? It's unclear to me given that the code is using pyproj. The local directory is now automatically created and populated with the INI file so that section can probably be removed. But if a more veteran person than I could have a look it would help.

Also I see an issue open about an installer. That may be something I can help with at some point in the future, but for the moment I am just trying to understand all the bits and pieces.

RadiosPRN commented 2 years ago

I am not sure what the exact status is at this time but I believe you are correct. GIS Internals and PDFtk are either not needed currently or should be unneeded shortly. Also pyproj needs to be added to the list of python modules that need to be installed.

You may want to look at the master-cj fork. There had been some work done on an installer (as well as quite a few other improvements) but those things were rolled back to a more stable version due to some bugs, with the hopes to incorporate some of those improvements back into the main branch at some point. I am not a coder (just know enough to get myself in trouble:) but have been using this software for about 2 years now and appreciate the work people have put into it.

caver456 commented 2 years ago

Hello erinn, thanks for finding radiolog. There is a good bit of history, I should make a timeline doc, but the short version is: this started as a passion project of mine after noticing that our SAR team's method of logging every radio transaction on paper ... could use some improvement.

So, we continue to use radiolog on 100% of our SAR calls, beginning around April 2015. BUT, I am not a programmer by trade so a LOT of it is not up to professional coding standards. Maybe the two biggest things to point out here are:

1) I am not terribly skilled at git / github, so please bear with me, and 2) there is no testing in place for radiolog - no unit tests, no regression tests, etc. This means, among other things, that testing is done by trying it out in our command trailer with as many scenarios as seem 'reasonable' - I'll hand-write a list of things to try, try them out, and if it's all good, will install the update on our SAR machines.

A brief timeline of the master vs master_cj branches: a volunteer SAR member from a different chapter spent a LOT of time and effort doing a major, deep-clean refactor of radiolog, but, since I didn't have any tests in place - he wrote a bunch of tests but I never followed up on his request to write some more test and frankly I still would not know how to do so for such a complex state machine like this - his overhauls caused some things to fail. He also planned to do more work on it but life changes prevented that from happening. Progress was stalled for a year or so because I wasn't sure of the best way to move forward in that situation. After hemming and hawing for a LONG time, I reverted to the version that was stable just prior to his refactoring, and moved his work to the master-cj branch. I do feel bad about that, and I think the root cause is that there are no tests in place in the master branch, and the tests in master-cj have big holes because I never followed up. Basically like @RadiosPRN says.

To your specific questions here:

An installer would be great. I futzed around with pyinstaller a bunch on the gpsio repo (also under ncssar) and it seems like pyinstaller is effectively a brick because some virus scanners will always flag it as a threat no matter what you do; so, I went with a nullsoft installer that installs the python embeddable (for windows) and then whatever else is in requirements.txt. If we could get radiolog to be installable in this same manner, that would win - largely so that folks can practice at home. Removing the GISInternals and pdftk dependencies were both intended to make this pure-python installation a bit easier.

BUT - if you've got experience there and can help make it happen that would win!

We just got back from a long hike, I am a bit too brain-fried to go farther now, and I will follow up on your other items in the next day or two. For now, thanks for finding radiolog and thanks for looking into collaboration!

erinn commented 2 years ago

Thanks for the history, context certainly helps. To be clear, I very much appreciate this work and very very much appreciate that it is open sourced for others to use. Our team is looking into fleetsync and this program is what came up so I started investigating it. The two PRs I've put in are fairly basic and benign, highly unlikely to break anything, but I am looking further into the code.

I currently have time on my hands and I think this is a valuable place to use it both for us as a team, for y'all as a team, but also for myself in terms of learning more about python. So if y'all are willing I'll start making some PRs and we can start building in some testing as well.

That being said here is my current focus, we as a team are testing fleetsync and wish to see it push through to sartopo. This could be a fairly simple standalone program that does this so I am looking to pull some of that code out of radiolog and create a standalone program that simply listens and pushes to SARtopo. As part of that work it makes sense to me, at least at this point, just to make the fleetsync work into a library which you can then import into radiolog. This'll cut down a bit on the 6k lines of code in that one file and is just considered better practice coding wise. If y'all are amenable to it, I'll hack that up and you folks can take a look at it, either which way I'll be creating the standalone program.

Beyond that, I know you are working on the PDF thing (and have a small suggestion later in this comment about that) and removing that dependency, well done! I don't really have a grasp on what the PDF stuff is for, I assume creating reports, but I am curious if there is an easier way so I'll have a poke at that. Much like the library work, this will come in the form of a PR (if I can even make it simpler, who knows) and y'all can take it or leave it as you see fit.

Finally, this is built on top of QT which provides several abstractions and convenience functions that are OS agnostic, for example, QSettings would seem to be a good candidate for holding settings in a platform independent way which could possibly alleviate the local_default -> local stuff that currently occurs with the INI file and would also allow for easier ability to configure those settings via the GUI itself. This is not my area of expertise but it was something that jumped out at me while I was going through the install process. This may be fruitful, it may not.

I've got a reasonable amount of python experience, but I am certainly no expert. I suppose the same can be said of git. Especially with these tools being used on Windows, I'm more of a Linux person myself.

Anyway, a small suggestion in terms of developing new features. Create a branch for the new feature, do your work and testing off of the branch, if everything appears stable then merge it into the main/master branch. This allows main/master to be 'stable' and anything you do can create whatever issues it creates without having an effect on main/master.

To put this in more concrete terms, the PDF work you are doing right now is checked into master/main, however the dependency is not listed (currently) in requirements.txt nor is it listed in the INSTALL.md file. Meaning I as a brand new user trying to install this from main must discern that you are doing PDF work on main, that it is not fully complete yet, and that I must do some extra intervention to make it work. Whereas if the PDF feature work was held in a branch it wouldn't matter at all, you could do all kinds of disruptive changes in the branch and master/main would continue on just fine. Once those disruptive changes were stable you merge the branch into main. If for some reason you find and issue after the merge you can always revert.

caver456 commented 2 years ago

All makes sense, looking forward to PRs and discussion.

Separating the fleetsync tasks out into a separate module or such sounds like a good idea. Modularity wins, I just never really knew that back when this started, and haven't much made a point to retrofit it into place. Have you checked out the sartopo_python repo in the same organizational account? Not sure if that's a good home for it, but feel free to assess and contribute on that one too. I'm the developer on that repo as well, so most of the same history and context applies.

I have never written or used a test, so I'm still largely uneducated about it and am in the dark about how one would write tests for complex state machine stuff - and honestly, whether it's a good use of time for this type of project, with a tiny, responsive, techie user base. It seems like there's a point of diminishing returns for complex behaviors, where the 'cost'/effort/pain of writing the next test begin to outweigh the incremental benefit of said test, and my impression is that point of diminishing returns would happen fairly quickly. But if you're already familiar with those questions and tradeoffs, and feel confident that it's the right way to go, then that's far more than I actually know about it!

Regarding PDFs - there are two cases: 1) when a clue is reported, an initial clue report form is generated as a PDF and printed as soon as the radiolog operator enters the data and saves the entry. 2) at the end of the operation, the clue log, full radio log, and team radio log (a separate radio log per callsign) are generated as PDFs and printed.

QSettings sounds promising, I never knew about that.

Making this all cross-platform would be good, in terms of potentially expanding the user base to teams that run on mac or linux.

For the branches, good point, I never really thought about doing it that way, though it's completely common sense now that you mention it. At this stage in the pdftk-removal work it probably makes sense to finish that out on the master branch (do you agree?) but after that, going forward, sideshow-branch-then-merge-when-ready sounds like a winner. That should also make it easier to deal with the ITTools repo also under ncssar, just in the early concept stages. Maybe you have some ideas on that repo too? Take a look at issue 1 there if you get the urge; but, not to sidetrack from the work that you are more motivated by.

caver456 commented 2 years ago

Also a bit more context, I used the term 'we' a few times but there are no other regular contributors (so far...) - just me.

erinn commented 2 years ago

for the pdftk work, yeah it makes sense just to continue as is with where you are. You've got a fairly small userbase and they can be flexible, but that git workflow mentioned just keeps things more stable. And if your userbase grows, it'll make them and you happier :).

Testing... oy, whole books can be written on this. It also seems to be a generational thing (to be broadly stereotypical). Younger folks want tests, it is 'the way' in their mind. Older folks tend to be a bit more balanced. What I can tell you is that the first time your test suite catches a bug you had no idea about, there is a large amount of delight in that. If I were in your position, I'd study up on a testing framework like pytest: https://www.oreilly.com/library/view/test-driven-development-with/9781491958698/ and implement tests going forward. A retrofit is going to be a large ask at this point. But whenever you touch code, think about developing tests for it. There are some hard hard things to build in testing, but testing is being done so often now that there are also some good libraries to assist with it. Also GitHub Actions can be a real saver here, you can use that to trigger tests on a PR creation for instance, meaning all PRs would have to have passing tests to be accepted, and you have to do no work to have this done after the initial setup. Anyway, we can deal with that further down the road.

radiolog is massively useful, granted it's useful to a small audience (SAR teams with Kenwood radios), but still. The better your testing framework is the more confidence you will have to make large changes to the code. As well, it will create a more robust application overall that more folks can use with high degrees of confidence that it 'works as advertised'.

caver456 commented 2 years ago

I am definitely the stubborn old fart in this scenario. I know I >should< be testing, but I have trouble getting as fired up and passionate about learning how to write tests for all the (seems like) trillions of obscure edge case scenarios - it's easier to get passionate about seeing an opportunity to improve the SAR workflow by throwing some lines of python together. Regardless - yes you/we can take up the testing thing on a different issue at some point.

Also, side note, Kenwood is not required to use radiolog - it certainly makes things a lot nicer and easier, but, fleetsync is the icing on the cake - it's not actually the cake. We do use radiolog outside the trailer every now and then, with different mobiles that aren't programmed up in the same manner (therefore we don't bother plugging them in to USB).

Back to this original issue: maybe it's best to clean up install.md after actually getting the install procedure together which could start now but couldn't be finalized until after I finish up the pdftk-removal? I hope to finish that up this weekend or next week. Or if you see a different workflow here, it's probably better than what I'm thinking of.

erinn commented 2 years ago

Yeah that makes sense to me. Like I said, small user base (I imagine), not many folks coming in like me clean. But I just wanted to point it out and start a discussion.

The only thing I see about PDFs, and I don't really understand exactly what you are doing with them, is the native OS support for printing to PDFs is usually very good. https://wiki.qt.io/Handling_PDF but this may not necessarily be 'better' in terms of time invested developing.

Because the choice was made to build on top of QT for this app there are a lot of powerful tools in QT that can really help out. As another example, you have code to poll the serial port after a set interval. Nothing wrong with that, but qtserialport module will hook into the native signals and slots mechanism (in theory, I am only starting to work with this) and as such a signal will can be emitted to the main loop when any data comes in on the serial port and an action (slot) can be taken in turn. This simplifies things, I think and uses your native tools. But again, I am just starting on work here and am basing this idea on theory alone.

I suppose the point is, look to QT for options because you are already hooked into that framework and those folks have done a LOT of work to make your life easier (not always of course).

I'll let you know what I find, you can also follow along with my slow progress here: https://github.com/erinn/ksync (dev branch). If any of this works out I'm happy to transfer it to y'all or not depending on what you want.

caver456 commented 2 years ago

Interesting - I never was aware that qt had serial port and pdf goodies. I bet that could have made life simpler from the start, but like you said it's hard to say if the benefits of porting over are big enough. Maybe so, maybe not, I don't really have an opinion there but it certainly points out the benefits of doing work in a different branch like you say.

The various pdfs that are generated:

The first three are generated by reportlab only. The last one was generated as an fdf plugged into a fillable pdf using pdftk, and the current plan is to use reportlab to generate the overlay, then use pypdf2 to overlay it on the non-fillable template pdf.

Radiolog generates the pdfs, and we do print them on every search as part of the paperwork stack that we give to the incident commander. Basically they really want to keep it all handy in case anything ever goes to court, so they have a policy to get all the paperwork for each search before we shut down.

caver456 commented 2 years ago

464 is fixed and closed. Please see #29 - this may all fall under that issue.