You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/05/10 18:09:27 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request, #6232: drivers/wireless: Change the enum of nrf24l01_datarate_t to the upper case
xiaoxiang781216 opened a new pull request, #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232
## Summary
## Impact
## Testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6232: tools/nxstyle: Add the suffix white list
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232#issuecomment-1126060433
> > Can we add more entries to the list? If I remember correctly we once changed a macro that had `mA` to `MA`, which is funny to say the least. :)
>
> Now we can add exception and rename back :)
Yes, I've found at least one file. I can submit a PR later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6232: drivers/wireless: Change the enum of nrf24l01_datarate_t to the upper case
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232#issuecomment-1125466527
We already have things like
```
# define SX127X_CMN_PARAMP_PARAMP_250us (4 << SX127X_CMN_PARAMP_PARAMP_SHIFT)
```
and
```
# define SX127X_CMN_PLL_BW_75kHz (0 << SX127X_CMN_PLL_BW_SHIFT)
```
in the code. And despite that `KHz` and `MHz` seem to be acceptable the `kHz` seems to be not. We can try to legitimize those cases via nxstyle exceptions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6232: drivers/wireless: Change the enum of nrf24l01_datarate_t to the upper case
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232#issuecomment-1124620521
If there is an exception for units (`Hz`) then probably we can add exception for other units like (`bps`, `us`, `ms`, `ns`). But that will mean that we will add exception for each new unit we introduce in code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6232: drivers/wireless: Change the enum of nrf24l01_datarate_t to the upper case
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232#issuecomment-1125852696
I think it would look weird if we uppercased all of the standard units and their prefixes. This will lead to confusions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6232: drivers/wireless: Change the enum of nrf24l01_datarate_t to the upper case
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232#issuecomment-1125872884
Ok, I add a suffix white list to handle these cases, please review again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6232: tools/nxstyle: Add the suffix white list
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232#discussion_r872380143
##########
tools/nxstyle.c:
##########
@@ -209,6 +209,14 @@ static const char *g_white_prefix[] =
NULL
};
+static const char *g_white_suffix[] =
+{
+ "kHz",
+ "Mbps",
+ "us",
Review Comment:
no macro end with us/ms, let's wait it happen before adding it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6232: tools/nxstyle: Add the suffix white list
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232#issuecomment-1126021484
Can we add more entries to the list?
If I remember correctly we once changed a macro that had `mA` to `MA`, which is funny to say the least. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko merged pull request #6232: tools/nxstyle: Add the suffix white list
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6232: drivers/wireless: Change the enum of nrf24l01_datarate_t to the upper case
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232#issuecomment-1124141482
Either way is fine for me, what's your opinion @pkarashchenko ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6232: drivers/wireless: Change the enum of nrf24l01_datarate_t to the upper case
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232#issuecomment-1125852695
I think it would look weird if we uppercased all of the standard units and their prefixes. This will lead to confusions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6232: tools/nxstyle: Add the suffix white list
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232#issuecomment-1126042499
> Can we add more entries to the list? If I remember correctly we once changed a macro that had `mA` to `MA`, which is funny to say the least. :)
Now we can add exception and rename back :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6232: tools/nxstyle: Add the suffix white list
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232#discussion_r872357859
##########
tools/nxstyle.c:
##########
@@ -209,6 +209,14 @@ static const char *g_white_prefix[] =
NULL
};
+static const char *g_white_suffix[] =
+{
+ "kHz",
+ "Mbps",
+ "us",
Review Comment:
do we need may `"ms"` and `"ns"` to be added as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6232: tools/nxstyle: Add the suffix white list
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6232:
URL: https://github.com/apache/incubator-nuttx/pull/6232#issuecomment-1126036498
> Can we add more entries to the list? If I remember correctly we once changed a macro that had `mA` to `MA`, which is funny to say the least. :)
all these macros end with MA now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org