search

Marzona / rig-remote

remote control a rig through rigctl protocol
MIT License
21 stars 6 forks source link

Synced with your devel branch and merged in new code #17

Closed MaineTim closed 8 years ago

MaineTim commented 8 years ago

Fixed a few stray issues with the way I had committed things. Fixed the code for frequency scan to handle threading, and hopefully there's nothing major broken by my changes at this point. Still have to implement "wait for signal" style pause and lockouts for bookmark scan. I had commented out the "activity notifications" as they conflicted with continuous scanning, but think selectable logging of all activity seen would be ideal. That's where the "log" checkbox is headed.

MaineTim commented 8 years ago

Update: wait-for-signal pause implemented, signal_check recoded to prevent some false positives I was having. Been running bookmark scans with it for a solid day and seems to perform well.

MaineTim commented 8 years ago

Have added bookmark lockout support and made the start/stop buttons into a single toggle. The lock button also toggles, though the text doesn't update. Locked bookmarks highlight in red, and this technique can be used to highlight scanning in general. Will add activity logging to a log file next.

Marzona commented 8 years ago

cool,

thanks for the work done.

Marzona commented 8 years ago

Hello,

checked the code and the features. It looks great! Thanks!.

Can I ask you to provide some unit tests for the code you are writing?

Marzona commented 8 years ago

This work seems to fall in: https://github.com/Marzona/rig-remote/issues/6

would you like to comment on that issue?

Marzona commented 8 years ago

actually, this merge broke 11 unit tests.

MaineTim commented 8 years ago

Hi Simone - My apologies, I did not run the test suite against the modified code. I've not used the pytest package. I don't think I changed any of the disk_io code, or any of the parameter entry checking code that you wrote, so that functionality should be the same. I haven't reviewed it yet, but if the test code depends on structures in the modified code, then it will have to be updated to reflect changes in those structures. I'll be glad to go over it, but won't be able to do so until tomorrow.

As to issue #6, the UI isn't blocking at this point, so full interactivity is possible. However, as coded right now, changes to parameters passed when the scanning thread is started aren't detected by the scanning thread, but they certainly could be, or a queue used to pass changes to the scanning thread.

MaineTim commented 8 years ago

Just took a quick look at test_scanning and it does look like the tests need to be updated to reflect the additional parameters in ScanningTask (scan_stop_button, passes, and probably log and wait). Have to head out to work, so won't be able to pursue it further for now.

Marzona commented 8 years ago

no hurry at all. when you've time, and thanks for taking a look to it so quickly. I'm going to set you as a contributor to avoid blocking you, and so to assign you the issues.

MaineTim commented 8 years ago

Sounds good. I'll close the fork tomorrow. On a side note, the readme file that I set up on the fork was copied over to this devel branch, so I just updated it to remove the fork references, but left the added features list so folks can see what's being worked on, if that works for you.

Marzona commented 8 years ago

looks good to me