Development Ideas

[Ricks-Lab]

I have opened this thread to facilitate any discussions on development ideas and code refinements. Potential topics:

• Recommendations for more Pythonic approach
• Gtk improvements for a nicer gui
• Better way to write to driver files as root
• Plotting capability approach
• Code criticism

[csecht]

When 'Save' is clicked in the PAC Gtk window and the sudo password is needed in the terminal, perhaps have the terminal window automatically come to the front, or perhaps have a notice pop up in the PAC window to prompt for an action in the terminal. As it is now, when the terminal window is behind the PAC window, it is not obvious for new users that terminal is waiting for sudo input.

[Ricks-Lab]

It's a good idea, though I will have to figure out how to not prompt the user after the credentials are entered the first time and still valid. Perhaps I can look at the delay to finish. It it is longer than a few seconds, the tell the user to check the window. I will also need to figure out how to construct a gtk dialog box...

[Ricks-Lab]

@dormiy @zhenyu-repo Any ideas?

I'm struggling with code to open a dialog before executing the batch file and then closing the dialog when complete. Below is code I was using to run the dialog in another thread. In the main thread, I use a dialogWin.close() call to close the dialog. It almost works. It displays the message, runs the bash file but gets error when running dialogWin.close() or hide or destroy. Here are error details:

[xcb] Unknown sequence number while processing queue
[xcb] Most likely this is a multi-threaded client and XInitThreads has not been called
[xcb] Aborting, sorry about that.
python3: ../../src/xcb_io.c:259: poll_for_event: Assertion `!xcb_xlib_threads_sequence_lost' failed.
[xcb] Unknown sequence number while processing queue
[xcb] Most likely this is a multi-threaded client and XInitThreads has not been called
[xcb] Aborting, sorry about that.
python3: ../../src/xcb_io.c:259: poll_for_event: Assertion `!xcb_xlib_threads_sequence_lost' failed.
Aborted (core dumped)

Here is the code:

def run_dialog(dialogWindow):
    dialogWindow.run()
    return


class PACDialog(Gtk.Dialog):
    def __init__(self):
        Gtk.Window.__init__(self, title="Execute PAC")
        message = "Executing bash script.  You may need to enter credentials in original window"
        dialogWindow = Gtk.MessageDialog(self,
                #Gtk.DialogFlags.MODAL | Gtk.DialogFlags.DESTROY_WITH_PARENT,
                Gtk.DialogFlags.DESTROY_WITH_PARENT,
                Gtk.MessageType.INFO,
                Gtk.ButtonsType.NONE,
                message)
        dialogWindow.set_title("Execute PAC")
        dialogWindow.show_all()
        #dialogWindow.run()
        p = multiprocessing.Process(target=run_dialog, args=(dialogWindow,))
        p.start()
        return

[Ricks-Lab]

I found that this code run after batch complete doesn't give any errors, but it looks like the destroy command never makes it to the dialog running in the new thread. The dialog doesn't close and the final join statement is stuck waiting for the process to finish.

   dialogWin.close()
   dialogWin.destroy()
   threadInfo["queue"].close()
   threadInfo["queue"].join_thread()
   threadInfo["p"].join()

[Ricks-Lab]

Maybe this is what I need:
Global Variable or Queue
I will give this a try when I have time.

[zhenyu-repo]

Maybe this is what I need:
Global Variable or Queue
I will give this a try when I have time.

Good luck, I'm new to Gtk, not sure what caused this. I guess you'll get it fixed this weekend 👍

[zhenyu-repo]

One feedback, I notice you have duplicate code blocks in multiple files, suggest moving them in one file/module and encapsulate them as a function. If you have similar design for other features, it would be better to refactor the same way.

    if args.about == True :  
        print(__doc__ )
        print("Author: ", __author__ )
        print("Copyright: ", __copyright__)
        print("Credits: ", __credits__)
        print("License: ", __license__)
        print("Version: ", __version__)
        print("Maintainer: ", __maintainer__)
        print("Status: ", __status__)
        sys.exit(0)

[Ricks-Lab]

@csecht Seems like you gave me quite a challenge with your apparently simple request. I still have not found a solution, but I have cleaned up my test cases into different branches:

• master is now stable version.
• threaded is the threaded message dialog approach - currently segfaults when terminating thread.
• message_box uses a text box at the bottom of the gui - currently doesn't update as I expect.

I may have to let my thoughts brew on this for a while and move on to working on some other things.

[Ricks-Lab]

One feedback, I notice you have duplicate code blocks in multiple files, suggest moving them in one file/module and encapsulate them as a function. If you have similar design for other features, it would be better to refactor the same way.

    if args.about == True :  
        print(__doc__ )
        print("Author: ", __author__ )
        print("Copyright: ", __copyright__)
        print("Credits: ", __credits__)
        print("License: ", __license__)
        print("Version: ", __version__)
        print("Maintainer: ", __maintainer__)
        print("Status: ", __status__)
        sys.exit(0)

Not sure if there is an easy way to do this as each of these metadata items are local to the file where they are defined and could be different in each file, though many could be the same. Not sure if I call a method in another module if it will use metadata as define in the calling module.

[Ricks-Lab]

Reddit saved the day! Solution for the message-box approach found:
PyGTK: How to update/redraw a Widget (GTKLabel)?

I hope I can clean up and release this approach over the weekend.

[Ricks-Lab]

When 'Save' is clicked in the PAC Gtk window and the sudo password is needed in the terminal, perhaps have the terminal window automatically come to the front, or perhaps have a notice pop up in the PAC window to prompt for an action in the terminal. As it is now, when the terminal window is behind the PAC window, it is not obvious for new users that terminal is waiting for sudo input.

@csecht I have completed the implementation of this feature. It is implemented in this branch
message_box Let me know what you think. Thanks for the idea.

[csecht]

@csecht I have completed the implementation of this feature. It is implemented in this branch
message_box Let me know what you think. Thanks for the idea.

Yes, that got my attention with the red highlighted message box at the bottom of the PAC window. Hard to miss! The use of that area for general information and status updates is a good idea too. Cheers.

[Ricks-Lab]

@csecht I am getting prepared to release v2.3.0. In addition to the amdgpu-pac message box, I have implemented checks of card compatibility throughout the reading of card details. Any card that has issues, will be flagged as incompatible and the pac and monitor tools will not use them. amdgpu-ls should still give the details it has for all AMD GPUs. I think this should make the tools much more robust. It would be great if you can give it a try and let me know of any issues. Thanks!

The latest is now on master.

[csecht]

I downloaded v2.3.0 from Master and see that now both -monitor and -pac no longer show the decoded model names in their windows, but instead show the short or display model names, so something broke there with this recent version. The correct preferred model name (Decoded Device ID) is still listed in amdgpu-ls, however (example from -ls);

Decoded Device ID: RX460
Card Model:  Baffin [Radeon RX 460/560D / Pro 450/455/460/555/560] (rev cf)
Short Card Model:  RX 460/560D / Pro 450/455/460/555/560
Display Card Model:  RX 460/560D / Pro 450/455/460/555/560

Also, it would be helpful in README, under the ## amdgpu-pac section, to mention that AUTO/default fan control can be restored by entering a negative integer as the % value. (I've always just used -1).

Everything else seems to work well with my RX 460 / RX 570 host. Looking good!
Cheers.