Tlm screen api #652

ghost · 2017-11-14T21:07:42Z

Create Api to get screen list and a specific screen definition

ghost

This was awesome in the demo you gave. Have you thought about how this will work with DART?

ghost · 2017-11-15T22:16:07Z

demo/config/tools/cmd_tlm_server/cmd_tlm_server.txt

@@ -42,4 +42,4 @@ BACKGROUND_TASK example_background_task.rb
  STOPPED
 BACKGROUND_TASK limits_groups.rb 5 # Initial delay to allow interfaces to connect

-COLLECT_METADATA
+#COLLECT_METADATA


Is there a reason this got commented out?

Every time I start CmdTlmServer it pops up and annoys me. I could be convinced to put it back as the default with a good argument.

I only mention because #655 found a bug in the basic install because we we NOT using it. As long as it's part of the AHK test I'm ok leaving it out.

ghost · 2017-11-15T22:19:50Z

lib/cosmos/system/system.rb

@@ -545,6 +485,91 @@ def self.load_configuration(name = nil)
      return self.instance.load_configuration(name)
    end

+    def reset_variables(filename = nil)


Please add comment block on the method

ghost · 2017-11-15T22:28:57Z

lib/cosmos/tools/cmd_tlm_server/cmd_tlm_server.rb

+      config_file = DEFAULT_CONFIG_FILE,
+      production = false,
+      disconnect = false,
+      mode = :CMD_TLM_SERVER)


Please update the comment block. I see create_message_log went away which apparently was never documented. Didn't we have some other ticket to remove that?

Also update comment blocks throughout. I'm not especially hard over on new simply methods but there are a bunch where you refactored things and now parameters don't match the method signature. I know, documentation ...

ghost · 2017-11-15T22:30:59Z

lib/cosmos/tools/cmd_tlm_server/cmd_tlm_server.rb


      # Set Threads to kill CTS if they throw an exception
      Thread.abort_on_exception = true

      # Don't start the DRb service or the telemetry monitoring thread
      # if we started the server in disconnect mode
      @json_drb = nil
-      start(production) unless @disconnect
+      unless @disconnect


Still feels like this giant block of code should be a separate method to me. Not sure why you inlined it.

ghost · 2017-11-15T22:33:07Z

lib/cosmos/tools/cmd_tlm_server/cmd_tlm_server.rb

+      if @reload_callback
+        @reload_callback.call(false) 
+      else
+        System.reset


Why is System.reset only called if there is NOT a reload_callback? How are these two states used? Please document.

ghost · 2017-11-15T22:40:16Z

lib/cosmos/tools/cmd_tlm_server/cmd_tlm_server_gui.rb

+      Qt.execute_in_main_thread(true) do
+        handle_string_output()
+        @output_sleeper.cancel
+        Qt::CoreApplication.processEvents()


How did this ever work outside of a execute_in_main_thread?

I believe it was only ever called from GUI context before these changes

ghost · 2017-11-15T22:45:46Z

lib/cosmos/tools/cmd_tlm_server/gui/interfaces_tab.rb

      @server_gui = server_gui
-      @interfaces_table = {}
+      @name = name


Save the name! Much better than passing it around. 👍

ghost · 2017-11-15T23:03:48Z

lib/cosmos/tools/cmd_tlm_server/gui/replay_tab.rb

+      @speed_select.connect(SIGNAL('currentIndexChanged(int)')) do
+        CmdTlmServer.replay_backend.set_playback_delay(@speed_select.itemData(@speed_select.currentIndex).value)
+      end
+      @speed_layout = Qt::FormLayout.new()


This comment applies throughout: do all these need to be instance variables? This one (for example) is only used here.

In older versions of qtbindings they did need to be instance variables to prevent getting garbage collected. In newer versions it isn't necessary but doesn't hurt anything.

Except for my eyes!

ghost · 2017-11-15T23:06:28Z

lib/cosmos/tools/cmd_tlm_server/replay_backend.rb

+
+    # Set the replay delay
+    #
+    # @param delay [Float] delay between packets in seconds 0.0 to 1.0, nil = No Delay, -1.0 = REALTIME


I think we need to document somewhere on the GUI that nil means No Delay and -1 means REALTIME. What's the difference between 0 delay and nil (No Delay)? Could nil be realtime and 0 be no delay? -1 for a value just isn't that intuitive.

In the GUI it is a combobox, so doesn't really need to be documented there. It just needs to be documented with the new API methods. I agree the values aren't very intuitive and we could make realtime = nil and no delay = 0. It would just change the code a bit. I'll look into it.

ghost · 2017-11-15T23:08:11Z

lib/cosmos/tools/data_viewer/data_viewer.rb

+      @replay_flag = Qt::Label.new("Replay Mode")
+      @replay_flag.setStyleSheet("background:green;color:white;padding:5px;font-weight:bold;")
+      @top_layout.addWidget(@replay_flag)
+      @replay_flag.hide


I love the way this Replay Mode notification in the GUI was done. 🥇

codecov-io · 2017-11-16T17:13:05Z

Codecov Report

Merging #652 into master will decrease coverage by 2%.
The diff coverage is 50.57%.

@@            Coverage Diff            @@
##           master    #652      +/-   ##
=========================================
- Coverage   81.11%   79.1%   -2.01%     
=========================================
  Files         150     153       +3     
  Lines       13658   14298     +640     
=========================================
+ Hits        11079   11311     +232     
- Misses       2579    2987     +408

Impacted Files	Coverage Δ
lib/cosmos/interfaces/protocols/fixed_protocol.rb	`100% <ø> (ø)`	⬆️
lib/cosmos/gui/qt.rb	`35.29% <0%> (-0.08%)`	⬇️
lib/cosmos/packets/structure.rb	`81.53% <100%> (ø)`	⬆️
lib/cosmos/packets/structure_item.rb	`77.33% <19.04%> (-6.88%)`	⬇️
lib/cosmos/tools/cmd_tlm_server/replay_backend.rb	`22.16% <22.16%> (ø)`
lib/cosmos/tools/tlm_viewer/tlm_viewer_config.rb	`18.27% <36.36%> (ø)`
lib/cosmos/tools/cmd_tlm_server/api.rb	`95.09% <48.93%> (-4.69%)`	⬇️
lib/cosmos/script/tools.rb	`93.18% <50%> (-4.32%)`	⬇️
lib/cosmos/script/cmd_tlm_server.rb	`86.13% <50%> (-2.29%)`	⬇️
lib/cosmos/script/replay.rb	`56% <56%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30bd935...99a130f. Read the comment docs.

coveralls · 2017-11-16T17:13:11Z

Coverage decreased (-2.04%) to 79.082% when pulling 2f4bbd5 on tlm_screen_api into 30bd935 on master.

coveralls · 2017-11-16T19:01:14Z

Coverage decreased (-2.04%) to 79.082% when pulling 99a130f on tlm_screen_api into 30bd935 on master.

ryanmelt added 9 commits October 25, 2017 10:45

start updating replay

57808c4

merge master

daaeb0e

continue replay changes

23f8716

tool replay mode

3c11763

continue replay changes

c7fa4d7

Merge branch 'master' into combine_replay

9c87cb1

replay api and gui

32dae96

ensure sort order

76b5f8e

create tlm screen api

3376479

ghost requested review from donaldatball and a user November 14, 2017 21:07

ryanmelt added 5 commits November 15, 2017 11:13

fix item specs

f659888

Merge branch 'master' into ensure_sort_order

7a7893a

fix leaving open file handle in put_into_archive

693d6d2

merge ensure_sort_order

1e357ec

fix specs

a18fa3b

ghost suggested changes Nov 15, 2017

View reviewed changes

ghost mentioned this pull request Nov 15, 2017

Combine replay #646

Merged

ryanmelt added 3 commits November 15, 2017 16:43

fix specs

681fca2

prevent calling start twice in CmdTlmServer.

abd673f

merge other branch

2f4bbd5

ryanmelt added 2 commits November 16, 2017 11:21

improve commenting, change delay values, address jasons comments

eae2b12

Merge branch 'combine_replay' into tlm_screen_api

99a130f

ghost approved these changes Nov 16, 2017

View reviewed changes

ghost merged commit 96d74e9 into master Nov 16, 2017

ghost deleted the tlm_screen_api branch November 16, 2017 20:54

ghost mentioned this pull request Nov 16, 2017

Add ability to query available screens from Telemetry Viewer #578

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tlm screen api #652

Tlm screen api #652

ghost commented Nov 14, 2017

ghost left a comment

ghost Nov 15, 2017

ghost Nov 16, 2017

ghost Nov 16, 2017

ghost Nov 15, 2017

ghost Nov 15, 2017

ghost Nov 15, 2017

ghost Nov 15, 2017

ghost Nov 15, 2017

ghost Nov 15, 2017

ghost Nov 16, 2017

ghost Nov 15, 2017

ghost Nov 15, 2017

ghost Nov 16, 2017

ghost Nov 16, 2017

ghost Nov 15, 2017

ghost Nov 16, 2017

ghost Nov 15, 2017

codecov-io commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

Tlm screen api #652

Tlm screen api #652

Conversation

ghost commented Nov 14, 2017

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 16, 2017 • edited Loading

Codecov Report

coveralls commented Nov 16, 2017 • edited Loading

coveralls commented Nov 16, 2017 • edited Loading

codecov-io commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading