Skip to content
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

Parameditor #635

Merged
merged 1 commit into from Aug 5, 2019

Conversation

@Akshath-Singhal
Copy link
Contributor

commented Jun 8, 2019

This is a GUI based parameter editor module which provides incremental or live filtering of parameters.

@tridge tridge added the WIP label Jun 8, 2019

@tridge

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

I just tried this and I can't see how to make it display any parameters. I just get this:
image
the read and refresh buttons don't work?

@Akshath-Singhal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

I just tried this and I can't see how to make it display any parameters. I just get this:
image
the read and refresh buttons don't work?

I will update that soon.

@Akshath-Singhal Akshath-Singhal force-pushed the Akshath-Singhal:parameditor branch 4 times, most recently from 5e8ab2a to ed69f24 Jun 10, 2019

@stephendade

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Just ran this on Windows 10 / Python 3. Loading the module gives this error:

AttributeError: 'ParamEditorMain' object has no attribute 'mavlink_packet'
Traceback (most recent call last):
  File "C:\Users\Stephen\AppData\Roaming\Python\Python36\site-packages\mavproxy-1.8.7-py3.6.egg\MAVProxy\modules\mavproxy_link.py", line 630, in master_callback
    mod.mavlink_packet(m)
  File "C:\Users\Stephen\AppData\Roaming\Python\Python36\site-packages\mavproxy-1.8.7-py3.6.egg\MAVProxy\modules\mavproxy_paraedit\__init__.py", line 33, in mavlink_packet
    self.pe_main.mavlink_packet(m)
MAVProxy/modules/mavproxy_paraedit/__init__.py Outdated Show resolved Hide resolved
MAVProxy/modules/mavproxy_paraedit/__init__.py Outdated Show resolved Hide resolved
MAVProxy/modules/mavproxy_paraedit/param_editor.py Outdated Show resolved Hide resolved
MAVProxy/modules/mavproxy_paraedit/param_editor.py Outdated Show resolved Hide resolved
MAVProxy/modules/mavproxy_paraedit/param_editor.py Outdated Show resolved Hide resolved
MAVProxy/modules/mavproxy_paraedit/param_editor.py Outdated Show resolved Hide resolved
MAVProxy/modules/mavproxy_paraedit/param_editor.py Outdated Show resolved Hide resolved
MAVProxy/modules/mavproxy_paraedit/param_editor.py Outdated Show resolved Hide resolved
MAVProxy/modules/mavproxy_param.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@tridge

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

I love the fast incremental search, that is great!
The green highlight for changed values is good too
as I'm sure you know, write doesn't work yet, and needs a refresh button for triggering a pull of the params.

@tridge

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

as you are clearly in the "broad brushstrokes" phase of developing this module I think I shouldn't be trying to do detailed feedback on the code as much of it will change as you fill out the features.
There are lots of features I'd like to see, but I'll wait till you get the basics in place then we should have a call to work out priorities of the next feature set

@Akshath-Singhal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

I love the fast incremental search, that is great!
The green highlight for changed values is good too
as I'm sure you know, write doesn't work yet, and needs a refresh button for triggering a pull of the params.

Actually I believe that the write works when "param" module is loaded, it does throw timeout message but I can observe the parameters being changed both in SITL and on a pixhawk. I am still trying to find out the reason for the timeout. I would like to discuss the difference between the functionality of refresh and read buttons as well.

@Akshath-Singhal Akshath-Singhal force-pushed the Akshath-Singhal:parameditor branch from 29eeb61 to 1b10d5a Jun 18, 2019

@peterbarker peterbarker self-requested a review Jun 19, 2019

@peterbarker
Copy link
Contributor

left a comment

If I set a integer parameter to "1.1" then the green never goes away (I used ACRO_LOCKING).

I suggest dropped the extra columns which aren't being filled with sensible data for the time being so we can get this committed. Don't lose the patches - we'd love to get those extra columns!

@stephendade

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

I'm getting this error when loading the module:
(Python 3.6 / Win10 / wxPython 4.0.6)

Traceback (most recent call last):
  File "C:\Python36\lib\multiprocessing\process.py", line 249, in _bootstrap
    self.run()
  File "C:\Python36\lib\multiprocessing\process.py", line 93, in run
    self._target(*self._args, **self._kwargs)
  File "C:\Users\Stephen\AppData\Roaming\Python\Python36\site-packages\mavproxy-1.8.7-py3.6.egg\MAVProxy\modules\mavproxy_paramedit\param_editor.py", line 146, in child_task
    parent=None, id=wx.ID_ANY)
  File "C:\Users\Stephen\AppData\Roaming\Python\Python36\site-packages\mavproxy-1.8.7-py3.6.egg\MAVProxy\modules\mavproxy_paramedit\param_editor_frame.py", line 50, in __init__
    self.Bind(wx.grid.EVT_GRID_CMD_CELL_CHANGE, self.ParamChanged,
AttributeError: module 'wx.grid' has no attribute 'EVT_GRID_CMD_CELL_CHANGE'
@peterbarker

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

@Akshath-Singhal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

I'm getting this error when loading the module:
(Python 3.6 / Win10 / wxPython 4.0.6)

Traceback (most recent call last):
  File "C:\Python36\lib\multiprocessing\process.py", line 249, in _bootstrap
    self.run()
  File "C:\Python36\lib\multiprocessing\process.py", line 93, in run
    self._target(*self._args, **self._kwargs)
  File "C:\Users\Stephen\AppData\Roaming\Python\Python36\site-packages\mavproxy-1.8.7-py3.6.egg\MAVProxy\modules\mavproxy_paramedit\param_editor.py", line 146, in child_task
    parent=None, id=wx.ID_ANY)
  File "C:\Users\Stephen\AppData\Roaming\Python\Python36\site-packages\mavproxy-1.8.7-py3.6.egg\MAVProxy\modules\mavproxy_paramedit\param_editor_frame.py", line 50, in __init__
    self.Bind(wx.grid.EVT_GRID_CMD_CELL_CHANGE, self.ParamChanged,
AttributeError: module 'wx.grid' has no attribute 'EVT_GRID_CMD_CELL_CHANGE'

That's strange. Does mission editor module run fine? It uses the same attribute as well.

@Akshath-Singhal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

If I set a integer parameter to "1.1" then the green never goes away (I used ACRO_LOCKING).

I suggest dropped the extra columns which aren't being filled with sensible data for the time being so we can get this committed. Don't lose the patches - we'd love to get those extra columns!

@peterbarker I have added xml file handling for filling the remaining columns. Please have a look.

@stephendade

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

That's strange. Does mission editor module run fine? It uses the same attribute as well.

The mission editor module's never successfully run under Windows, but that's a separate (multiprocessing related) issue.

I've run this on my Ubuntu laptop with the same bug. Some digging has revealed a Python2/3 issue:
-Runs fine under Python 2.7
-Crashes with the aforementioned error under Python 3.6 (both under Windows and Ubuntu)

I've confirmed I'm running the same wxPython version under both Python environments.

Am I the only person running Python 3 here? :)

EDIT:
Turns out I was running wxPython 3 on Python 2.7 and wxPython 4 on Python 3.5 ... so it's a version conflict somewhere

@Akshath-Singhal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

That's strange. Does mission editor module run fine? It uses the same attribute as well.

The mission editor module's never successfully run under Windows, but that's a separate (multiprocessing related) issue.

I've run this on my Ubuntu laptop with the same bug. Some digging has revealed a Python2/3 issue:
-Runs fine under Python 2.7
-Crashes with the aforementioned error under Python 3.6 (both under Windows and Ubuntu)

I've confirmed I'm running the same wxPython version under both Python environments.

Am I the only person running Python 3 here? :)

EDIT:
Turns out I was running wxPython 3 on Python 2.7 and wxPython 4 on Python 3.5 ... so it's a version conflict somewhere

Seems like wxPython 4 has different attributes and would require wx.grid.GridEvent.EVT_GRID_CELL_CHANGING instead.

@tridge

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

notes from call discussion:

  • should update param values when PARAM_VALUE packet seen, without new Read
  • display editor form of checkboxes and selection values always in options, not just check double-clicked
  • update value column on each change in bitmask and selection, not just on escape key
  • if possible display scroll bar all the time for scrolling param window
  • rename Options heading to Selection
  • for parameters which have an Increment and Range, use a scrolly/increment widget
  • remove number column for parameters
  • don't display more than 1 digit past the increment in float display
  • rename Reset to "Reset to Defaults". Make it pop up a "Are you sure you want to reset all parameters to defaults yes/no" dialog box.
  • remove select XML file button
  • give timeout error display when can't write parameter
  • change cell to orange background when write is pending

errors:
(mavproxy.py:14914): Gtk-WARNING **: 14:32:06.033: Error loading theme icon 'document-save' for stock: Icon 'document-save' not present in theme elementary-xfce-darkest

(mavproxy.py:14914): Gtk-CRITICAL **: 14:42:23.293: gtk_widget_set_allocation: assertion '_gtk_widget_get_visible (widget) || _gtk_widget_is_toplevel (widget)' failed

python3:

MANUAL> Loaded module paramedit
Process Process-2:
Traceback (most recent call last):
File "/usr/lib/python3.6/multiprocessing/process.py", line 258, in _bootstrap
self.run()
File "/usr/lib/python3.6/multiprocessing/process.py", line 93, in run
self._target(*self._args, **self._kwargs)
File "/home/tridge/.local/lib/python3.6/site-packages/MAVProxy-1.8.7-py3.6.egg/MAVProxy/modules/mavproxy_paramedit/param_editor.py", line 155, in child_task
parent=None, id=wx.ID_ANY)
File "/home/tridge/.local/lib/python3.6/site-packages/MAVProxy-1.8.7-py3.6.egg/MAVProxy/modules/mavproxy_paramedit/param_editor_frame.py", line 57, in init
self.Bind(wx.grid.EVT_GRID_CMD_CELL_CHANGE, self.ParamChanged,
AttributeError: module 'wx.grid' has no attribute 'EVT_GRID_CMD_CELL_CHANGE'

py3 crash on edit value:
Reloaded module paramedit
/home/tridge/.local/lib/python3.6/site-packages/MAVProxy-1.8.7-py3.6.egg/MAVProxy/modules/mavproxy_paramedit/checklisteditor.py:80: wxPyDeprecationWarning: Using deprecated class. Use GridCellEditor instead.
/home/tridge/.local/lib/python3.6/site-packages/MAVProxy-1.8.7-py3.6.egg/MAVProxy/modules/mavproxy_paramedit/checklisteditor.py:15: wxPyDeprecationWarning: Using deprecated class. Use GridCellEditor instead.
Traceback (most recent call last):
File "/home/tridge/.local/lib/python3.6/site-packages/MAVProxy-1.8.7-py3.6.egg/MAVProxy/modules/mavproxy_paramedit/checklisteditor.py", line 90, in Create
IndexError: list index out of range

@Akshath-Singhal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Tasks as per call discussion

  • should update param values when PARAM_VALUE packet seen, without new Read
  • display editor form of checkboxes and selection values always in options, not just check double-clicked
  • update value column on each change in bitmask and selection, not just on escape key
  • display scroll bar all the time for scrolling param window
  • rename Options heading to Selection
  • For parameters which have an Increment and Range, use a scrolly/increment widget
  • remove number column for parameters
  • don't display more than 1 digit past the increment in float display
  • rename Reset to "Reset to Defaults". Make it pop up a "Are you sure you want to reset all parameters to defaults yes/no" dialog box.
  • remove select XML file button
  • give timeout error display when can't write parameter
  • change cell to orange background when write is pending

The list will be updated as new tasks are added for more features or when some of these tasks are performed.

@peterbarker

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

@Akshath-Singhal there are unaddressed review comments. Scroll down through the "Files Changed" tab and address each in turn, please. If you don't think it's a real problem that's OK (we can argue about it :-) ), but you should address each.

@peterbarker

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

The "open file" dialogue box seems to be saying it's restricting to ".wp" files (maybe a hold-over from the waypoint editor?):

image

... OTOH, it isn't so I'm really not sure what's going on there...

@peterbarker

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

"Read" resets parameters to values from the autopilot bit fields retain their green colour (indicating they've been changed).

Incidentally, we use red in the waypoint editor to indicate unsynced (in some status text at the top)

@Akshath-Singhal Akshath-Singhal force-pushed the Akshath-Singhal:parameditor branch from 5c23d3f to 16352fd Jul 18, 2019

@Akshath-Singhal

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

The "open file" dialogue box seems to be saying it's restricting to ".wp" files (maybe a hold-over from the waypoint editor?):

image

... OTOH, it isn't so I'm really not sure what's going on there...

Yeah. It's a holdover from mission editor and has been corrected.

@Akshath-Singhal

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

The "Selections" column isn't populated until I click in it:

image

That's because there doesnt exist cell render for chechlistbox and dropdown menu. Would you like it to display the selected value or the editors as well? Displaying the selected values is easier but custom cell renders might take time.

@tridge

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

if you load param module at startup with --load-module paramedit then it freezes and is unkillable

@peterbarker

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

Dropdown values appear to be incorrect (0.3 vs 3)

image

@peterbarker
Copy link
Contributor

left a comment

Please run flake8 on modified files and ensure no new issues have been introduced.

Closing the paramedit window should unload the module

@tridge

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

selection box is showing wrong value, it is showing index
image

@peterbarker

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

Sometimes a dropdown, sometimes a +/- thing?

image

@peterbarker

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

Future enhancement: tickbox for "show verbose description" - if not checked then we only show the short description so we get more parameters shown.

@Akshath-Singhal Akshath-Singhal force-pushed the Akshath-Singhal:parameditor branch from 00d4686 to 588dbf8 Jul 18, 2019

@Akshath-Singhal

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

selection box is showing wrong value, it is showing index
image

Corrected

@stephendade

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

Just tested this on Win10 / Python 3.7. The module still crashes on startup

HOLD> module load paramedit
HOLD> ERROR in command ['load', 'paramedit']: handle is closed
Traceback (most recent call last):
  File ".\MAVProxy\mavproxy.py", line 600, in process_stdin
    fn(args[1:])
  File ".\MAVProxy\mavproxy.py", line 416, in cmd_module
    load_module(modname, **kwargs)
  File ".\MAVProxy\mavproxy.py", line 366, in load_module
    module = m.init(mpstate, **kwargs)
  File "C:\Users\Stephen\AppData\Roaming\Python\Python37\site-packages\mavproxy-1.8.8-py3.7.egg\MAVProxy\modules\mavproxy_paramedit\__init__.py", line 38, in init
    return ParamEditorModule(mpstate)
  File "C:\Users\Stephen\AppData\Roaming\Python\Python37\site-packages\mavproxy-1.8.8-py3.7.egg\MAVProxy\modules\mavproxy_paramedit\__init__.py", line 23, in __init__
    self.pe_main = param_editor.ParamEditorMain(mpstate)
  File "C:\Users\Stephen\AppData\Roaming\Python\Python37\site-packages\mavproxy-1.8.8-py3.7.egg\MAVProxy\modules\mavproxy_paramedit\param_editor.py", line 101, in __init__
    self.child.start()
  File "C:\Program Files (x86)\Python37-32\lib\multiprocessing\process.py", line 112, in start
    self._popen = self._Popen(self)
  File "C:\Program Files (x86)\Python37-32\lib\multiprocessing\context.py", line 223, in _Popen
    return _default_context.get_context().Process._Popen(process_obj)
  File "C:\Program Files (x86)\Python37-32\lib\multiprocessing\context.py", line 322, in _Popen
    return Popen(process_obj)
  File "C:\Program Files (x86)\Python37-32\lib\multiprocessing\popen_spawn_win32.py", line 89, in __init__
    reduction.dump(process_obj, to_child)
  File "C:\Program Files (x86)\Python37-32\lib\multiprocessing\reduction.py", line 60, in dump
    ForkingPickler(file, protocol).dump(obj)
  File "C:\Program Files (x86)\Python37-32\lib\multiprocessing\connection.py", line 948, in reduce_pipe_connection
    dh = reduction.DupHandle(conn.fileno(), access)
  File "C:\Program Files (x86)\Python37-32\lib\multiprocessing\connection.py", line 170, in fileno
    self._check_closed()
  File "C:\Program Files (x86)\Python37-32\lib\multiprocessing\connection.py", line 136, in _check_closed
    raise OSError("handle is closed")
OSError: handle is closed
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "C:\Program Files (x86)\Python37-32\lib\multiprocessing\spawn.py", line 105, in spawn_main
    exitcode = _main(fd)
  File "C:\Program Files (x86)\Python37-32\lib\multiprocessing\spawn.py", line 115, in _main
    self = reduction.pickle.load(from_parent)
EOFError: Ran out of input

@Akshath-Singhal Akshath-Singhal force-pushed the Akshath-Singhal:parameditor branch from 588dbf8 to 0733114 Jul 18, 2019

@tridge

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

displaying with too many decimals:
image

@tridge

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

100% cpu usage on simple test:

  • load paramedit module
  • change ACRO_RP_P using increment widget
  • press write
  • cell goes orange
  • cpu goes to 100%
    image

@tridge tridge force-pushed the Akshath-Singhal:parameditor branch from 346fc9b to c91f974 Jul 31, 2019

@tridge

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

tab completion on module load shows both 'paraedit' and paranedit' ?

MAV> module load p
paraedit   paramedit  ppp        
@tridge

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

getting crashes on write:

(mavproxy.py:26230): Gtk-WARNING **: 18:46:47.759: for_size smaller than min-size (26 < 32) while measuring gadget (node entry, owner GtkSpinButton)

(mavproxy.py:26230): Gtk-WARNING **: 18:46:47.776: for_size smaller than min-size (26 < 32) while measuring gadget (node entry, owner GtkSpinButton)

(mavproxy.py:26230): Gtk-WARNING **: 18:46:47.793: for_size smaller than min-size (26 < 32) while measuring gadget (node entry, owner GtkSpinButton)
Received 1067 parameters
Saved 1067 parameters to mav.parm
*** BUG ***
In pixman_region32_init_rect: Invalid rectangle passed
Set a breakpoint on '_pixman_log_error' to debug

@Akshath-Singhal Akshath-Singhal force-pushed the Akshath-Singhal:parameditor branch 2 times, most recently from 2e9438a to d9581c4 Aug 4, 2019

Added GUI Parameter Editor
Added GUI based parameter editor for read, write and reset of parameters with custom editors for bitmask checkbox, value dropdown and increment control. GTK warning only is displayed only if moddebug is set to 3 or higher.

@Akshath-Singhal Akshath-Singhal force-pushed the Akshath-Singhal:parameditor branch from d9581c4 to 6f76ee2 Aug 5, 2019

@Akshath-Singhal

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Please run flake8 on modified files and ensure no new issues have been introduced.

Closing the paramedit window should unload the module

@peterbarker Done. I used a little different method as compared to what you have done in misseditor but it seems to work fine. I am able to unload and load modules multiple times and only one instance of the module can be opened at a time.

@Akshath-Singhal

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

tab completion on module load shows both 'paraedit' and paranedit' ?

MAV> module load p
paraedit   paramedit  ppp        

@tridge Can you try to pull the changes on a new branch?

@tridge tridge merged commit 9b95331 into ArduPilot:master Aug 5, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.