Skip to content

CurieSoftwareSerial Improvements #101

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

Closed
wants to merge 1 commit into from

Conversation

bigdinotech
Copy link
Contributor

-renamed global variable rxPin to _rxPin
-allow different baud rates for different instances of a SoftwareSerial object
-minor code cleanup of unused code

@bigdinotech
Copy link
Contributor Author

@kmsywula @SidLeung ready for code review

rxPin = _receivePin;
_rxPin = _receivePin;
rxIntraBitDelay = _rx_delay_intrabit;
rxCenteringDelay = _rx_delay_centering;

Choose a reason for hiding this comment

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

Is there a good reason for defining any global variables here? This essentially makes all of these variables "reserved words" for anyone who includes this library in their sketch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I used globals is because recv() which acts as the interrupt handler is static.
Therefore, it cannot access those variables if I kept them as member variables of the class.

Do you think having those variables names "reserved" can be an issue? If so I can always rename them to something else.

Also if you have a better solution, Pull Requests are always welcome. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

If they are internal and only to be used inside the library, the convention is to precede name with _ character

Also if you made them static then scope is limited to the file they are defined in.

Choose a reason for hiding this comment

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

just make them static.

Copy link
Contributor

Choose a reason for hiding this comment

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

One other way, equally not pretty, to allow static ISR routine to access variable of an object is,

  1. Define the ISR as private in a Class.
  2. Instantiate a static object of the Class.
  3. Define a static function, ISR_wrapper, that the sole purpose is to call the ISR of the Class via the static object.
    The only advantage here is that the ISR is now free to access all variables in the Class without making them global, static, etc.

@soundanalogous
Copy link

Using an unnamed namespace may also work (but I haven't tried this within an ISR that could be a limitation). Wrap all of the global variables (and it also works for functions) with:

namespace {
  // declare your variables here
  // ...
}

The compiler will create a unique namespace. There is nothing else you'd need to change. Here's an article that explains this approach: http://www.comeaucomputing.com/techtalk/#nostatic

@SidLeung
Copy link
Contributor

Mmm... May be not. ISR is not the direct cause. The fact that the ISR has to be declared as a static routine and it needs to access data/variables in a class that has not been instantiated, the compiler get upset. Static function can only access static variable...

-changed global variables into static
-allow different baud rates for different instances of a SoftwareSerial object
-minor code cleanup of unused code
-moved source code into src directory
@bigdinotech
Copy link
Contributor Author

@kmsywula @SidLeung please review again

@SidLeung
Copy link
Contributor

@calvinatintel Looks good. All global are static.

@calvinatintel
Copy link
Contributor

Cherry-picked

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.

7 participants