-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GUI v9.0 #28
GUI v9.0 #28
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember you and MMJ mentioned that the compilation will also be fixed in this PR?
launchUI.py
Outdated
@@ -669,7 +669,7 @@ def leave(event): | |||
def clickTile(ID): | |||
widgets["fuConfigPannel"].config(text='Tile '+str(ID)+' functional units') | |||
widgets["xbarConfigPannel"].config(text='Tile '+str(ID)+' crossbar outgoing links') | |||
widgets["xbarConfigPannel"].grid(columnspan=4, row=7, column=0,rowspan=2) | |||
widgets["xbarConfigPannel"].grid(columnspan=4, row=7, column=0,rowspan=2,sticky="nsew") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment here about the meaning of "nsew"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In this sentence "nsew" means that each xbar component will fill the corresponding grid space. Similar usage is seen in the subsequent code.
|
||
spmLabel = tkinter.Button(canvas, text = "Data\nSPM", fg = 'black', bg = 'gray', relief = 'raised', bd = BORDER, command = clickSPM) | ||
#button.place(height=memHeight, width=MEM_WIDTH, x = 25, y = 35) | ||
canvas.create_window(baseX+BORDER, BORDER, window=spmLabel, height=GRID_HEIGHT, width=MEM_WIDTH, anchor="nw") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment explaining the anchor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment on
anchor
.
Didn't see how this is resolved? Once you resolve a comment, can you please reply in this dialogue window (instead of just closing it), so it is more clear to the reviewer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I forgot this one. Here I use anchor="nw" to express that the data memory will be placed in the upper left corner of the cgrapannel. This way, it will be adaptive to changes in window size.
launchUI.py
Outdated
widgets["spmConfigPannel"] = spmConfigPannel | ||
for i in range(3): | ||
spmConfigPannel.rowconfigure(i,weight=1) | ||
|
||
spmConfigPannel.columnconfigure(0,weight=2) | ||
spmConfigPannel.columnconfigure(0,weight=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment here about what are the columnconfigure
used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Here the columnconfigure means that the spmConfigPannel will be evenly divided into three columns, and each column will fill the corresponding grid space.
Hi Mr.Tan. I added comments to the code and modified the corresponding statements. I reviewed the execution method of launchUI.py in the ReadME file and I found that as long as I follow the steps in the original ReadME file, cd build, then python /WORK_REPO/CGRA-Flow/launchUI.py, the compile app function can be used normally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, which kernel you tried to compile? It also failed on my machine/docker... (I also create build
folder and typed python ../launchUI.py
. Or do you mean we need to do python /WORK_REPO/CGRA-Flow/launchUI.py
?
Hi Mr.Tan.I have fine-tuned the initial size of the GUI to reduce redundant space. I adjusted the relative size of several functional modules and moved the tile's positions. For SPM, there were no y-axis coordinates in the initial setup so I chose to extend the height of SPM to accommodate different scales of CGRA. To ensure visual aesthetics, the size of the mapping section is not adjusted. I hope the adjustment is proper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screenshot looks great to me. Just some nit to be fixed.
launchUI.py
Outdated
paramPannel.columnconfigure(i,weight=1) | ||
paramPannel.rowconfigure(i, weight=1) | ||
for i in range(3): | ||
paramPannel.columnconfigure(i, weight=1) # Use columnconfigure and rowconfigure to partition the columns, so that each column and row will fill the corresponding space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move comment above the code.
paramPannel.rowconfigure(i, weight=2) | ||
for i in range(4): | ||
paramPannel.columnconfigure(i,weight=1) | ||
paramPannel.rowconfigure(i, weight=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please comment on weight
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Here the entire paramPannel is divided into many rows and columns, and the weights are allocated according to the size of the components to be occupied. The “weight” represents the weight of the corresponding row/column length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, i mean add the comment above the corresponding code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I will do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were some issues with my local environment, and a clash occurred, leading to an inability to sync the changes. I will submit the modified version in the future.
|
||
spmLabel = tkinter.Button(canvas, text = "Data\nSPM", fg = 'black', bg = 'gray', relief = 'raised', bd = BORDER, command = clickSPM) | ||
#button.place(height=memHeight, width=MEM_WIDTH, x = 25, y = 35) | ||
canvas.create_window(baseX+BORDER, BORDER, window=spmLabel, height=GRID_HEIGHT, width=MEM_WIDTH, anchor="nw") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment on
anchor
.
Didn't see how this is resolved? Once you resolve a comment, can you please reply in this dialogue window (instead of just closing it), so it is more clear to the reviewer?
Sure, please let me do once the environment issue is fixed so we can merge. |
@@ -1566,8 +1566,9 @@ def place_xbar_options(master): | |||
def create_param_pannel(master): | |||
paramPannel = tkinter.LabelFrame(master, text='Configuration', bd=BORDER, relief='groove') | |||
paramPannel.grid(row=0, column=1, rowspan=1, columnspan=1, sticky="nsew") | |||
|
|||
# Use columnconfigure and rowconfigure to partition the columns, so that each column and row will fill the corresponding space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: period is missing at the end of comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I will add that period.
Hi, Mr Tan. I realized the format needs to be consistent so after checking all the added comments, I have unified them into a form without periods. I hope this time the file will look more fluid and aesthetically pleasing so that the subsequent merge and refinement can be more smooth. |
SG. Is the environment issue fixed on your side and we are ready to merge this PR? |
Yes. Yesterday the lab's server encountered a DNS pollution fault. I modified the host configuration file and now the environment on my side is back to normal. |
Cool, thanks! |
Hi Mr.Tan.I added scrollbars to the cgraPannel, aligned the xbarButton and fuCheckButton in the parampannel module, and made some slight adjustments to the GUI to make it more useful."