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

Improve rock hz #30

Merged
merged 3 commits into from
Dec 8, 2016
Merged

Improve rock hz #30

merged 3 commits into from
Dec 8, 2016

Conversation

doudou
Copy link
Member

@doudou doudou commented Nov 22, 2016

Depends on rock-core/package_set#106

This fixes the way rock-hz was doing its averaging (which was really not working well), and adds the --host option.

In addition, it improves the UI:

  • without arguments, or if the given task/port do not match an existing
    one, the script now lists matching tasks/ports and lets the user
    choose one. This is currently buggy if there is more tasks than
    one page, but IMO it's still much better than the current situation
    of having to figure out task and port names and type them.
  • the display uses the TTY gem instead of hardcoding ANSI
    codes, and formats the output as a table without having a running
    console
  • with the -c COUNT option, it shows a history of the values

one_s = compute_frequency(now, one_s_queue, 1)
ten_s = compute_frequency(now, ten_s_queue, 10)

# Terminal code to go up 1 line and clear the line
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is still there, but the corresponding code not.
I would want to keep the in place update for the terminal as it was before

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What sucks with the in-place update is that one does not see how the frequency progresses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like the running console - and even for your purpose, if you want to display and analyse the frequency progress, then why not use a better readable output format or log file that can readily be used for gnuplot?

@@ -30,57 +38,53 @@ if !(options[:task_name] || options[:port_name])
exit
end

require 'orocos'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loading orocos after the options block has been done due to the fact that loading takes some time and waiting for the help message for few seconds does not give a good user experience.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not give a good user experience.

.... I'll refrain from commenting on that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you actually run time ruby -e "require 'orocos'" on your machine ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First time load and 2 subsequent ones: Ubuntu 14.04, ruby2.0
ruby -e "require 'orocos'" 0.74s user 0.15s system 37% cpu 2.374 total
$ time ruby -e "require 'orocos'"
ruby -e "require 'orocos'" 0.46s user 0.04s system 98% cpu 0.508 total
$ time ruby -e "require 'orocos'"
ruby -e "require 'orocos'" 0.44s user 0.05s system 99% cpu 0.495 total

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ruby -e "require 'orocos'" 0.74s user 0.15s system 37% cpu 2.374 total

So, cold cache, never loaded orocos.rb, 2.3 seconds. I doubt that rock-hz is really going to be the very first orocos-related command you'll run on a fresh machine. Then 0.5 seconds. Is that really worth duplicating every single command-line option in an option hash to then apply them ?

This adds a few features to rock-hz:
 - without arguments, or if the given task/port do not match an existing
   one, the script now lists matching tasks/ports and lets the user
   choose one. This is currently buggy if there is more tasks than
   one page, but IMO it's still much better than the current situation
   of having to figure out task and port names and type them.
 - the display uses the TTY gem instead of hardcoding ANSI
   codes, and formats the output as a table without having a running
   console
 - with the -c COUNT option, it shows a history of the values
@doudou
Copy link
Member Author

doudou commented Nov 28, 2016

Pushed some other improvements. It reestablishes the non-running console. Given that the loading time of require 'orocos' is 0.5 seconds with a hot disk cache, I really don't see the point of complexifying everything.

@2maz
Copy link
Member

2maz commented Dec 8, 2016

K, thx. Looks fine to me.

@doudou doudou merged commit 411d89d into master Dec 8, 2016
@doudou doudou deleted the improve_rock_hz branch December 8, 2016 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants