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

Implement libcec4 compatibility #300

Merged
merged 1 commit into from Nov 12, 2017

Conversation

Projects
None yet
4 participants
@psyke83

psyke83 commented Nov 11, 2017

Needed for Raspbian stretch.

So far, only compile-tested, no build warnings. Unfortunately, when I try to hold a remote button to configure input, "CEC" appears grayed and disappears; it seems that my TV's CEC implementation doesn't support long-press, so the code may need changing to work for me.

pinging @tomaz82

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 Nov 11, 2017

Collaborator

@psyke83 Was the duplication of the actual callback functions needed? They look identical to me, and it
seems that the older libcec also had support for the cec_adapter_descriptor, currently testing it, if that's the case then it's only the 4 lines that sets the callbacks the needs to be inside an ifdef.

Collaborator

tomaz82 commented Nov 11, 2017

@psyke83 Was the duplication of the actual callback functions needed? They look identical to me, and it
seems that the older libcec also had support for the cec_adapter_descriptor, currently testing it, if that's the case then it's only the 4 lines that sets the callbacks the needs to be inside an ifdef.

@psyke83

This comment has been minimized.

Show comment
Hide comment
@psyke83

psyke83 Nov 11, 2017

Yes, it was necessary. Look more closely - the duplicated functions are now void type and some of the arguments are now pointers, which also needed changes to your onKeyPress code's key.duration and key.keycode.

psyke83 commented Nov 11, 2017

Yes, it was necessary. Look more closely - the duplicated functions are now void type and some of the arguments are now pointers, which also needed changes to your onKeyPress code's key.duration and key.keycode.

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 Nov 11, 2017

Collaborator

@psyke83 Oh yeah, I see it now, but at least the cec_adapter_descriptor should work on both libcec versions, testing it as we speak but my TV also has dodgy cec, it works sometimes, and sometimes not, and it's not es, even cec-client fails to read any input when this happends.

Collaborator

tomaz82 commented Nov 11, 2017

@psyke83 Oh yeah, I see it now, but at least the cec_adapter_descriptor should work on both libcec versions, testing it as we speak but my TV also has dodgy cec, it works sometimes, and sometimes not, and it's not es, even cec-client fails to read any input when this happends.

@psyke83

This comment has been minimized.

Show comment
Hide comment
@psyke83

psyke83 Nov 11, 2017

I'll switch over to jessie and confirm that for you (re: cec_adapter_descriptor) & update the PR once I'm confirmed it still builds ok.

psyke83 commented Nov 11, 2017

I'll switch over to jessie and confirm that for you (re: cec_adapter_descriptor) & update the PR once I'm confirmed it still builds ok.

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 Nov 12, 2017

Collaborator

I just confirmed that your change to using cec_adpater_descriptor, DetectAdapters and strComName all works on jessie, I also figured out why my cec was dodgy, it doesn't read input if I launch ES remotely from putty, only if I launch it on the actual Pi.

So, only the callbacks, and the setting of the callbacks needs to be inside ifdef

Collaborator

tomaz82 commented Nov 12, 2017

I just confirmed that your change to using cec_adpater_descriptor, DetectAdapters and strComName all works on jessie, I also figured out why my cec was dodgy, it doesn't read input if I launch ES remotely from putty, only if I launch it on the actual Pi.

So, only the callbacks, and the setting of the callbacks needs to be inside ifdef

@psyke83

This comment has been minimized.

Show comment
Hide comment
@psyke83

psyke83 Nov 12, 2017

Good, will update. I assume you won't mind if I update your debug logging to use "DetectAdapters" and "strComName" too.

psyke83 commented Nov 12, 2017

Good, will update. I assume you won't mind if I update your debug logging to use "DetectAdapters" and "strComName" too.

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 Nov 12, 2017

Collaborator

Yeah, that's what I tried to say, keep your code, no need to have the old code in ifdef ( the descriptor, DetectAdapters and DebugLog calls ) your new code work fine on jessie as well, it's only the callback and the setup of them that needs both old a new inside ifdef

Collaborator

tomaz82 commented Nov 12, 2017

Yeah, that's what I tried to say, keep your code, no need to have the old code in ifdef ( the descriptor, DetectAdapters and DebugLog calls ) your new code work fine on jessie as well, it's only the callback and the setup of them that needs both old a new inside ifdef

@psyke83

This comment has been minimized.

Show comment
Hide comment
@psyke83

psyke83 Nov 12, 2017

I understood, don't worry. I deliberately didn't change the function names in the debug logging, even in the ">=4" part (which I'll now remove) to keep the logs consistent, but it's better to update the debugging to use the accurate names, since it'll be the same for all versions.

psyke83 commented Nov 12, 2017

I understood, don't worry. I deliberately didn't change the function names in the debug logging, even in the ">=4" part (which I'll now remove) to keep the logs consistent, but it's better to update the debugging to use the accurate names, since it'll be the same for all versions.

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 Nov 12, 2017

Collaborator

Oh, I broke it tho, the path debug is wrong, it's using strComName for both, wth.., give me 1 sec

Collaborator

tomaz82 commented Nov 12, 2017

Oh, I broke it tho, the path debug is wrong, it's using strComName for both, wth.., give me 1 sec

@psyke83

This comment has been minimized.

Show comment
Hide comment
@psyke83

psyke83 Nov 12, 2017

It should be .strComPath?

psyke83 commented Nov 12, 2017

It should be .strComPath?

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 Nov 12, 2017

Collaborator

Yeah, just found the correct header, damn that took way too long time :P

Collaborator

tomaz82 commented Nov 12, 2017

Yeah, just found the correct header, damn that took way too long time :P

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 Nov 12, 2017

Collaborator

And change the log to write out "name: " ? and not strComName ? same for path, I guess.

LOG(LogDebug) << "adapter: " << i << " path: " << adapters[i].strComPath << " name: " << adapters[i].strComName;

Collaborator

tomaz82 commented Nov 12, 2017

And change the log to write out "name: " ? and not strComName ? same for path, I guess.

LOG(LogDebug) << "adapter: " << i << " path: " << adapters[i].strComPath << " name: " << adapters[i].strComName;

@psyke83

This comment has been minimized.

Show comment
Hide comment
@psyke83

psyke83 Nov 12, 2017

OK. Will you be happy with this?

LOG(LogDebug) << "adapter: " << i << " path: " << adapters[i].strComPath << " name: " << adapters[i].strComName;

psyke83 commented Nov 12, 2017

OK. Will you be happy with this?

LOG(LogDebug) << "adapter: " << i << " path: " << adapters[i].strComPath << " name: " << adapters[i].strComName;
@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 Nov 12, 2017

Collaborator

Yeah, exactly!

Collaborator

tomaz82 commented Nov 12, 2017

Yeah, exactly!

@psyke83

This comment has been minimized.

Show comment
Hide comment
@psyke83

psyke83 Nov 12, 2017

Should be ready now.

OT: I tried reducing the HOLD_TIME in es-core/src/guis/GuiDetectDevice.cpp to help enter the input configuration, but I still can't skip entries, hah. I guess I won't be using CEC in ES without manually editing a controller configuration.

psyke83 commented Nov 12, 2017

Should be ready now.

OT: I tried reducing the HOLD_TIME in es-core/src/guis/GuiDetectDevice.cpp to help enter the input configuration, but I still can't skip entries, hah. I guess I won't be using CEC in ES without manually editing a controller configuration.

@joolswills joolswills merged commit e94fb5c into RetroPie:master Nov 12, 2017

@joolswills

This comment has been minimized.

Show comment
Hide comment
@joolswills

joolswills Nov 12, 2017

Member

Thanks.

Member

joolswills commented Nov 12, 2017

Thanks.

@psyke83

This comment has been minimized.

Show comment
Hide comment
@psyke83

psyke83 Nov 12, 2017

Thanks.

@tomaz82 I've tested CEC now... it works well upon first look, and when returning from launching an emulator, but Kodi seems to have major issues:

  • Kodi will freeze when loading its CEC plugin, or else Kodi will run but CEC will not function.
  • Exiting Kodi either causes Kodi itself to stay stuck indefinitely on the shutdown menu, or it will close, but EmulationStation segfaults.

On Jessie, Kodi uses libcec4, so I tried removing libcec3, libcec-dev and rebuilt ES with libcec4, so that the same library version is used for both programs. CEC still works OK in ES but doesn't improve the interaction with Kodi.

I noticed that when ES launches an item, "lsof" still shows libcec loaded into ES. I removed libcec from the common libraries (https://github.com/RetroPie/EmulationStation/blob/master/CMakeLists.txt#L183), but it doesn't help. I suspect that the library may not be not closing CEC/unloading correctly.

psyke83 commented Nov 12, 2017

Thanks.

@tomaz82 I've tested CEC now... it works well upon first look, and when returning from launching an emulator, but Kodi seems to have major issues:

  • Kodi will freeze when loading its CEC plugin, or else Kodi will run but CEC will not function.
  • Exiting Kodi either causes Kodi itself to stay stuck indefinitely on the shutdown menu, or it will close, but EmulationStation segfaults.

On Jessie, Kodi uses libcec4, so I tried removing libcec3, libcec-dev and rebuilt ES with libcec4, so that the same library version is used for both programs. CEC still works OK in ES but doesn't improve the interaction with Kodi.

I noticed that when ES launches an item, "lsof" still shows libcec loaded into ES. I removed libcec from the common libraries (https://github.com/RetroPie/EmulationStation/blob/master/CMakeLists.txt#L183), but it doesn't help. I suspect that the library may not be not closing CEC/unloading correctly.

@psyke83 psyke83 deleted the psyke83:cec4fix branch Nov 12, 2017

@pjft

This comment has been minimized.

Show comment
Hide comment
@pjft

pjft Nov 12, 2017

That behavior seems to happen when Kodi detects some types of devices plugged in on start or exit. Are you unloading CEC when launching Kodi and an emulator, and reloading it when coming back to ES? Could that be an option?

pjft commented Nov 12, 2017

That behavior seems to happen when Kodi detects some types of devices plugged in on start or exit. Are you unloading CEC when launching Kodi and an emulator, and reloading it when coming back to ES? Could that be an option?

@psyke83

This comment has been minimized.

Show comment
Hide comment
@psyke83

psyke83 Nov 12, 2017

Looking at Tomas' patch, CECInput::deinit() gets called in InputManager::deinit(), which is called before an item is launched through Window::deinit(), so theoretically, ES in its suspended state shouldn't be interfering with Kodi.

Even when I manually disable the Kodi's CEC plugin, it doesn't resolve the freezing and/or segfault issue.

When ES is running, I see:

pi@retropie:~/RetroPie-Setup $ lsof | grep cec
emulation 25235                pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
emulation 25235 25236          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
VCHIQ     25235 25237          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
HDispmanx 25235 25238          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
HTV       25235 25239          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
HCEC      25235 25240          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
emulation 25235 25241          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
emulation 25235 25242          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
emulation 25235 25243          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2

When I launch an emulator, some, but not all references are gone:

pi@retropie:~/RetroPie-Setup $ lsof | grep cec
emulation 25235                pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
emulation 25235 25236          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
VCHIQ     25235 25237          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
HDispmanx 25235 25238          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
HTV       25235 25239          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
HCEC      25235 25240          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2

This is output from a build where libcec is removed from common libraries. This does make a difference, as if I comment out CECInput::init(), libcec won't appear in ES maps (but will if it's part of the common libraries).

P.S. The Pi/Broadcom-related processes that appear are using libcec due to ES; when ES is killed, no processes at all reference libcec.

psyke83 commented Nov 12, 2017

Looking at Tomas' patch, CECInput::deinit() gets called in InputManager::deinit(), which is called before an item is launched through Window::deinit(), so theoretically, ES in its suspended state shouldn't be interfering with Kodi.

Even when I manually disable the Kodi's CEC plugin, it doesn't resolve the freezing and/or segfault issue.

When ES is running, I see:

pi@retropie:~/RetroPie-Setup $ lsof | grep cec
emulation 25235                pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
emulation 25235 25236          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
VCHIQ     25235 25237          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
HDispmanx 25235 25238          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
HTV       25235 25239          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
HCEC      25235 25240          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
emulation 25235 25241          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
emulation 25235 25242          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
emulation 25235 25243          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2

When I launch an emulator, some, but not all references are gone:

pi@retropie:~/RetroPie-Setup $ lsof | grep cec
emulation 25235                pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
emulation 25235 25236          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
VCHIQ     25235 25237          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
HDispmanx 25235 25238          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
HTV       25235 25239          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2
HCEC      25235 25240          pi  mem       REG      179,2   593832       1929 /usr/lib/arm-linux-gnueabihf/libcec.so.4.0.2

This is output from a build where libcec is removed from common libraries. This does make a difference, as if I comment out CECInput::init(), libcec won't appear in ES maps (but will if it's part of the common libraries).

P.S. The Pi/Broadcom-related processes that appear are using libcec due to ES; when ES is killed, no processes at all reference libcec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment