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

Several improvements and fixes #14

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

guicamest
Copy link

  • Reduce lines read from cec-client
  • Faster conflicting process checking
  • Some cec keys are not emitted
  • Don't break if lines in retroarch are in different order
  • Allow remote key repeats (by holding them down)
  • Allow configuration of cec mappings via cfg file

Copy link

@Forceu Forceu left a comment

Choose a reason for hiding this comment

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

Appears to be a lot more responsive! Just a sidenote though, if the script finds an unknown key the non-existent get_keymap function is being called.

Also PullRequest #11 should definitely be merged with this one.

@guicamest
Copy link
Author

Hi @Forceu ! Glad you liked it. I've been using it for a few days now and so far no problems.
What do you mean by "if the script finds an unknown key the non-existent get_keymap function is being called"?
Considering that there are a lot of key mappings (remote control-libcec-controller-retroarch-keyboard-uinput) I don't know what you mean by "unknown key".
@Forceu @dillbyrne let me know which PR is merged first

@Forceu
Copy link

Forceu commented Mar 22, 2018

I meant the function in line 117 of your PR

If an unknown key like "non-standard-key" is found (instead of eg. "enter" or "f1"), it would call the function get_keymap. However you replaced get_keymap() with get_keyboard_to_uinput_map().

@dillbyrne is this project still maintained? If not, I forked it and incorporated the PRs: https://github.com/Forceu/es-cec-input

@dillbyrne
Copy link
Owner

Hi @guicamest @Forceu . Thanks for the PRs 👍 . I won't have a chance until sometime next week to test the PRs as I'm away from the TV / Pi. I'll merge them then if there are no issues.

@guicamest
Copy link
Author

@Forceu Thanks for pointing that out! Just pushed a fix for it.
@dillbyrne Sure no problem!

@guicamest
Copy link
Author

Hey @dillbyrne , did you have a time to look at this?

@dillbyrne
Copy link
Owner

dillbyrne commented Mar 31, 2018

Hi @guicamest the proposed changes add too much cpu overhead considering the specs of the pi. Considering what the script does, it should only use very little cpu and only when it is in use.

htop output

script without changes

memory is 0.9
cpu while kodi is active 0.0
cpu while idle in ES is 0.0
cpu while active in ES is 1.4 from pressing a key
cpu while stressed in ES is around 9.0 from non stop pressing. Drops immediately once pressing has ceased

script with proposed changes

memory is 0.9
cpu while kodi is active 7.6/9 It should 0.0 when kodi is active
cpu while idle in ES is 8.1/9.8
cpu while active in ES is 9.5 from pressing a key
cpu while stressed in ES is around 9.5

I would look at the main loop and subprocess code as it is running in kodi when it shouldn't be. This means that it will be adding ~9% overhead in emulators too which we obviously can't have.

@Forceu
Copy link

Forceu commented Apr 2, 2018

I can confirm that I have the same behaviour on my Pi. I first thought it was related to the Wifi AP I added, but came to the conclusion that it is actually the script. My Pi heated up to 82°C in idle.

@guicamest
Copy link
Author

I didn't notice anything unusual about cpu usage but I'll take a look at it again. Thanks for the insight!

    * Reduce lines read from cec-client
    * Faster conflicting process checking
    * Some cec keys are not emitted
    * Don't break if lines in retroarch are in different order
    * Allow remote key repeats (by holding them down)
    * Allow configuration of cec mappings via cfg file
@guicamest
Copy link
Author

@dillbyrne @Forceu Found the issue! In other to process keys as they are pressed in the remote, the loop was invoked every 0.002 seconds, therefore it made a lot of calls to ps(to check running processes). To avoid that a sleep would do, but we'd be missing the ability to process keys as they are pressed in the remote.
Basically I separated ps checking into another thread, sleeping 3 seconds per call( start time of kodi in my Pi is ~5s )
They main loop blocks for incoming events, therefore almost no cpu is used.
From what I managed to see, the script now uses 1% maximum( when a remote key is hold down)

@guicamest
Copy link
Author

@dillbyrne @Forceu any updates?

2 similar comments
@guicamest
Copy link
Author

@dillbyrne @Forceu any updates?

@guicamest
Copy link
Author

@dillbyrne @Forceu any updates?

es-cec-input.py Outdated
with open(ra_cfg, 'r') as fp:
for line in fp:
<<<<<<< 9ea7e255376d2db0c22efcedfa097b5a640977fd

Choose a reason for hiding this comment

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

There is a conflict starting this line @guicamest

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the heads up @paradadf ! It's been like this for more than a year... :|
Already fixed it and updated :)

Choose a reason for hiding this comment

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

@guicamest, I wanted to contact you about other things in this improvements you did but couldn’t find a way. Are somewhere on discord or how could I write you a line? (Sorry, I know this isn’t the place)

Copy link
Author

Choose a reason for hiding this comment

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

Feel free to comment on the relevant commit: https://github.com/guicamest/es-cec-input/commits/pressed

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.

4 participants