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

[PVR] Initialize timer info tag with first available timer type from client #15882

Merged
merged 1 commit into from Apr 12, 2019

Conversation

Projects
None yet
2 participants
@djp952
Copy link
Contributor

commented Apr 9, 2019

Initialize timer info tag with first timer type provided by the PVR client, when available, instead of arbitrarily trying to create a manual one-shot timer type which may not be supported and causes unnecessary errors in the log.

Description

During construction, the CPVRTimerInfoTag class will attempt to default to a Manual One-Shot timer from the PVR client. If creation fails, it will fall back to trying to create whatever the first available timer type from any PVR client happens to be.

If the PVR client does not support Manual One-Shot timers, the Kodi log will have a great deal of "PVR::CPVRTimerType::CreateFromAttributes: Unable to resolve timer type (0x1, 0xa, xxxxxxxx)" errors in it, one for every time a CPVRTimerInfoTag instance is created.

While there may have been a historic reason for defaulting to Manual One-Shot, it doesn't appear necessary any longer, making the error messages false-positive. If Manual One-Shot isn't available, the code already falls back to a default type and continues without error.

The proposed change removes the arbitrary creation of a Manual One-Shot timer and instead, since the PVR client ID is known, retrieves the first available timer type for that client. The CPVRTimerType::GetFirstAvailableType() method was modified to use the client ID instead of making a new method since this is the only place it's referenced.

Motivation and Context

This change eliminates the plethora of false positive errors present in the Kodi logs if a PVR client does not support Manual One-Shot timers. It should have no effect on the operation of the PVR subsystem since it was already falling back into similar code. The original fallback didn't filter at all (for example PVR_TIMER_TYPE_FORBIDS_NEW_INSTANCES), the proposed change also does not.

How Has This Been Tested?

Primary testing was on Windows 10, x64 and Win32 using my custom PVR (which does not support Manual One-Shot timers), with secondary testing on Android ARM64 (nVidia Shield).

I had set breakpoints on the offending error message and ran through unit testing of timers -- initial load, add/modify/delete. All instances were generated from the CPVRTimerInfoTag constructor. Ran the same unit testing after the modifications and ran into no issues with the same PVR timer operations (add/modify/delete), and verified that the message no longer appeared in the logs.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@djp952 djp952 changed the title PVR: Initialize timer info tag with first available timer type from client [PVR] Initialize timer info tag with first available timer type from client Apr 9, 2019

Show resolved Hide resolved xbmc/pvr/timers/PVRTimerType.h
return types.empty() ? CPVRTimerTypePtr() : *(types.begin());
}
}
return CPVRTimerTypePtr();

This comment has been minimized.

Copy link
@ksooo

ksooo Apr 11, 2019

Member

'return {};'

This comment has been minimized.

Copy link
@ksooo

ksooo Apr 11, 2019

Member

I would suggest to rewrite this like:

const CPVRTimerTypePtr CPVRTimerType::GetFirstAvailableType(int iClientId)
{
  const CPVRClientPtr client = CServiceBroker::GetPVRManager().GetClient(iClientId);
  if (client)
  {
    std::vector<CPVRTimerTypePtr> types;
    if (client->GetTimerTypes(types) == PVR_ERROR_NO_ERROR && !types.empty())
      return *(types.begin());
  }
  return {};
}
return allTypes.empty() ? CPVRTimerTypePtr() : *(allTypes.begin());
std::vector<CPVRTimerTypePtr> types;
const CPVRClientPtr client = CServiceBroker::GetPVRManager().GetClient(iClientId);
if(client)

This comment has been minimized.

Copy link
@ksooo

ksooo Apr 11, 2019

Member

blank between 'if' and '('

const CPVRClientPtr client = CServiceBroker::GetPVRManager().GetClient(iClientId);
if(client)
{
if(client->GetTimerTypes(types) == PVR_ERROR_NO_ERROR)

This comment has been minimized.

Copy link
@ksooo

ksooo Apr 11, 2019

Member

blank between 'if' and '('

@djp952

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Thanks @ksooo, all requested changes have been made. Rebuilt and (lightly) unit tested.

For consistency in the .cpp file, I also replaced the other two instances of "return CPVRTimerTypePtr()" with "return {};", I'm guessing you would prefer them to all match, LMK if not and I'll undo those.

@ksooo

ksooo approved these changes Apr 12, 2019

Copy link
Member

left a comment

Thank you.

@ksooo ksooo added the v18 Leia label Apr 12, 2019

@ksooo ksooo added this to the Leia 18.2-rc1 milestone Apr 12, 2019

@ksooo ksooo merged commit 36bf9e0 into xbmc:master Apr 12, 2019

1 check passed

default You're awesome. Have a cookie
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.