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

Change channel type in event module #40

Merged
merged 3 commits into from
Jan 16, 2022
Merged

Change channel type in event module #40

merged 3 commits into from
Jan 16, 2022

Conversation

pjueon
Copy link
Owner

@pjueon pjueon commented Jan 13, 2022

This is a fix for the issue #39.
(Summary: The channel type in event module should be changed because it is NOT always convertible to int. )
Tested it with samples/test_all_apis.cpp.

@ShimmyShaman
Could you review the code please?

Major changes:

  • The signature of wait_for_edge, add_event_detect functions changed
  • The signature of the callback changed

Want to discuss:

  • Any better idea for the return value of the wait_for_edge function?
    • it returns 0 on timeout if the channel is int.
    • it returns an empty string on timeout if the channel is string.

- CVM and TEGRA_SOC use strings instead of numbers.
So the channel is NOT always convertible to int.
- in wait_for_edge, add_event_detect function,
std::atoi is used to convert the channel to int .
If the user is using CVM orTEGRA_SOC,
these functions will throw an exception.
- see #39
@pjueon pjueon changed the title Fix channel type in event module Change channel type in event module Jan 13, 2022
@pjueon
Copy link
Owner Author

pjueon commented Jan 14, 2022

I came up with some options for the return type of wait_for_edge function:

Option 1. Use different types for each overloading function (now)

// return the channel for which the edge was detected. 
//If a timeout occurred, it returns 0(if the channel is int) or an empty string(if the channel is string).
std::string wait_for_edge(const std::string& channel, Edge edge, unsigned long bounce_time = 0, unsigned long timeout = 0);
int wait_for_edge(int channel, Edge edge, unsigned long bounce_time = 0, unsigned long timeout = 0);

//...


// in the user code,
if (wait_for_edge(...) == channel)
// or
// for string channel
if(!wait_for_edge(...).empty())

// for int channel
if(wait_for_edge(...))
// ...

Option 2. create a GPIO::WaitResult just for it.

class WaitResult
{
public:
   WaitResult(const std::string& channel) : _channel(channel){}
   
   bool is_event_detected() const { return !channel().empty(); };
   operator bool() const { return is_event_detected(); }

   // can get the channel info
   const std::string& channel() const { return _channel; }

private:
   std::string _channel;
};

// both versions return GPIO::WaitResult object
WaitResult wait_for_edge(const std::string& channel, unsigned long bounce_time = 0, unsigned long timeout = 0);
WaitResult wait_for_edge(int channel, Edge edge, unsigned long bounce_time = 0, unsigned long timeout = 0);

// ...

// in the user code, 
if(wait_for_edge( .... ).is_event_detected())
// or just
if(wait_for_edge( .... ))
// ...

//...
auto result = wait_for_edge( .... );
std::string channel = result.channel(); 

Option 3. always return std::string

// return empty string on timeout
 std::string wait_for_edge(const std::string& channel, Edge edge, unsigned long bounce_time = 0, unsigned long timeout = 0);
 std::string wait_for_edge(int channel, Edge edge, unsigned long bounce_time = 0, unsigned long timeout = 0);

// in the user code,
if(!wait_for_edge(...).empty())

@ShimmyShaman
Which one do you think is the best? Personally, option 2 is my favorite.

@ShimmyShaman
Copy link
Collaborator

ShimmyShaman commented Jan 15, 2022

Option 2 is a nice solution.
All the code changes look functional and non-breaking. You've got my approval for what you've done.

@pjueon
Copy link
Owner Author

pjueon commented Jan 15, 2022

Thank you for the review.
Alright, I'll implement option 2 and merge it when I get back home.

- add GPIO::WaitResult class
- change the return type of wait_for_edge to WaitResult
- update README.md
@pjueon pjueon merged commit 67ae500 into master Jan 16, 2022
@pjueon pjueon deleted the channel_type branch January 16, 2022 11:17
@pjueon pjueon added the bug Something isn't working label Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants