-
Notifications
You must be signed in to change notification settings - Fork 583
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
A set of minor fixes #2044
A set of minor fixes #2044
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me, just few question on why some pin limitations. Maybe a small comment is enough if there is this limit. May help for maintenance.
{ | ||
_sequence = new List<byte>() | ||
{ | ||
(byte)command | ||
}; | ||
} | ||
|
||
internal FirmataCommandSequence(FirmataCommand command, int pin) | ||
{ | ||
if (pin > 15) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all the boards with the same limit? If I'm using an ESP32 I can get more. So did I miss here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a limit of the command, not of the protocol or the board. There are some commands that have a one-byte shorthand form, but this form is limited to 15 pins, because the pin number is encoded in a nibble only. All such commands have a longer form as well, see the destinction below.
int pwmMaxValue = _supportedPinConfigurations[pin].PwmResolutionBits; // This is 8 for most arduino boards | ||
pwmMaxValue = (1 << pwmMaxValue) - 1; | ||
int value = (int)Math.Max(0, Math.Min(dutyCycle * pwmMaxValue, pwmMaxValue)); | ||
|
||
// At most 14 bits used? | ||
if (((pwmMaxValue & 0x3FFF) == pwmMaxValue) && (pin <= 15)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, why the 15 pins limits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment: If the pin number is <=15, a shorthand form of the command can be used.
This PR includes a set of minor fixes for problems found while using some bindings.
Microsoft Reviewers: Open in CodeFlow