search

twVolc / PyCamPermanent

Permanent PiCam (SO2) installation project software
GNU General Public License v3.0
2 stars 2 forks source link

Issue 140/preloaded cal rtp #193

Closed twVolc closed 3 weeks ago

twVolc commented 2 months ago

Catches preloaded calibration selection when starting RTP and raises and error to the GUI highlighting that this is invalid.

Also raises a warning if preloaded calibration is selected but no calibration file has been defined.

twVolc commented 2 months ago

@ubdbra001 It's been a while since I solved merge conflicts. Your help would be appreciated! (This isn't a priority, I've just been working through some straightforward issues to try to help out).

I made this branch yesterday so I'm guessing the RTP stuff changed a bit of this code. I'm not totally clear on how it gets resolved. It looks like save_calibration() maybe no longer has a check on cal_type_int? If that's the case I'm guessing we can ignore my edits to that part of the code completely? The only reason I edited this is to move away from checking if calibration_series exists and instead check if it is None. There was one place in the code that was already checking if it was None, so this hit an error if no series had been defined at that point as the attribute didn't exist. I think in reality that check for None was looking to do the same thing that other checks were doing through hasattr(self, 'calibration_series'), so to standardise things I've moved away from the hasattr() checks and into predefining calibration_series in __init__() as None, and just always checking this instead. If there's a reason you preferred deleting the attribute and checking whether it exists then I'm happy to go with that, but I just thought it might be a bit clearer to always have the attribute exist and set it to None when it is missing.

ubdbra001 commented 2 months ago

I'll take a look at this on Monday to work out how to fix the conflict. Thanks!

ubdbra001 commented 1 month ago

Getting around to having a look at this now, and I think we should be avoiding creating GUI elements in the backend elements, e.g. in so2_camera_processor.py:

 messagebox.showwarning('Must load calibration',
                        'Warning! Preloaded calibration is selected but no '
                        'calibration file has been loaded. Please select a file to '
                        'load to enable calibration.')

These will present a problem for GUI-less running as I'm not sure if/how a user would be able to deal with them.

twVolc commented 1 month ago

Yes I agree with this and think I thought it at the time but just wanted a quick fix. I couldn't see a straightforward way of raising this kind of issue to the GUI, which is definitely a useful thing to have when working with the GUI. One solution I thought about, which I assumed would be quite quick and easy to implement when we work to make the code run GUI-less, is just have a flag in PyplisWorker that says whether it's in GUI-less or GUI running mode. If in GUI-less it doesn't raise this message box warning and instead logs/prints a warning. I think there's a few places where GUI pop-ups are currently generated in PyplisWorker, but I'd have thought finding these and adding this check should be quite quick when we think we're ready for GUI-less. What do you think?

ubdbra001 commented 1 month ago

I think it would be good to avoid adding GUI code to PyplisWorker just to avoid any issues around GUI-less running.

We want this check to be done in both with and without GUI running so it will need to be in the PyplisWorker code, so maybe have it raise an exception or return a value that is then caught by the GUI code to show the error box?

twVolc commented 1 month ago

Raising an exception that's caught by the GUI very much sounds like the proper way to go about this!

ubdbra001 commented 1 month ago

Raising an exception that's caught by the GUI very much sounds like the proper way to go about this!

I think so too, but we'd have to work out how that would work without the GUI running: the exception would be thrown and if there's nothing to catch it then it will just crash PyCam. From my understanding the desired behaviour would be: don't start watching, warn the user, and let them try again?

twVolc commented 1 month ago

Yep something like that. It does feel like we're going to need some kind of flag for if we're in GUI or GUI-less running in PyplisWorker and IFitWorker, where they do slightly different things with certain errors depending on that state. Errors that might need to be raised for the GUI probably just need printing and/or logging in GUI-less.

In some respects though, if running in GUI-less, crashing the the software perhaps isn't the worst thing. My thinking of how GUI-less will work is that you set it up with a config file at the start and it just runs - no interactions with a user after this, so no way of changing options without restarting the software. In this case, raising an error that crashes the software isn't an issue - it tells the user that their config file must be set up incorrectly. They change the config file, run PyCam again, and hopefully it will run. Certain errors (i.e. bugs) in GUI-less will want logging and the software just restarting itself, but I think user errors like this one should only be encountered on start-up and crashing the software seems like a reasonable outcome.

ubdbra001 commented 1 month ago

Okay, that sounds reasonable.

I'll start with the sensible approach of raising an exception in PyplisWorker that is handled in the GUI to produce a message box, and see how that goes.

ubdbra001 commented 3 weeks ago

This has been superseded by #219, may be worth closing