You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by "jakkra (via GitHub)" <gi...@apache.org> on 2023/02/06 08:43:49 UTC
[GitHub] [mynewt-newtmgr] jakkra opened a new pull request, #193: Allow setting the UART MTU from arguments to utilize larger serial pa…
jakkra opened a new pull request, #193:
URL: https://github.com/apache/mynewt-newtmgr/pull/193
…cket in order to speed up flashing speeds with MCUBoot.
Default setting is not changed, but now the mtu argument to `--connstring=COM55,baud=1000000,mtu=512` will be used.
Currently it seems as the mtu parameter was not used for serial, possibly making this a bug fix?
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow setting the UART MTU from arguments to utilize larger serial pa… [mynewt-newtmgr]
Posted by "davidtaylor-juul (via GitHub)" <gi...@apache.org>.
davidtaylor-juul commented on code in PR #193:
URL: https://github.com/apache/mynewt-newtmgr/pull/193#discussion_r1572844874
##########
nmxact/nmserial/serial_xport.go:
##########
@@ -50,7 +50,7 @@ var errTimeout error = errors.New("Timeout reading from serial connection")
func NewXportCfg() *XportCfg {
return &XportCfg{
ReadTimeout: 10 * time.Second,
- Mtu: 512,
+ Mtu: 128,
Review Comment:
Dropping a comment here for anyone else who may have found this was a breaking change in some cases.
This _does_ change the default value that is fed into `transceiver.go`, which does packet fragmentation at the layer above the serial transport (the CoAP layer). The value is read in the `MtuOut` method in [serial_sesn.go#L187](https://github.com/apache/mynewt-newtmgr/blob/cf9985f71c7fab6a71f345ab55438a7d58dc6a3f/nmxact/nmserial/serial_sesn.go#L187)
We had to set the MTU provided to `transceiver.go` to "unlimited" to prevent it from fragmenting the CoAP payload, and instead allow the `serial_xport.go` `TxCoap` method to fragment based on the actual MTU.
I'm not sure if there should be a broader fix for that, as it does seem redundant to have fragmentation at the CoAP layer, and again in the transport layer. At least over serial, fragmentation at the CoAP layer breaks re-assembly on the receiver end. Maybe other transports rely on the CoAP layer fragmenting and do not do transport layer fragmentation? I don't know, but in our use case it is necessary to disable the fragmentation in `transceiver.go:TxCoap` for serial connections.
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-newtmgr] bridadan commented on pull request #193: Allow setting the UART MTU from arguments to utilize larger serial pa…
Posted by "bridadan (via GitHub)" <gi...@apache.org>.
bridadan commented on PR #193:
URL: https://github.com/apache/mynewt-newtmgr/pull/193#issuecomment-1634575705
Just as a heads up, when using this with Zephyr, I had to add some configs to make use of an MTU higher than 128. Without the configs, the image upload times out:
```
# Greater than or equal to largest MTU
CONFIG_BOOT_MAX_LINE_INPUT_LEN=512
# Equal to BOOT_MAX_LINE_INPUT_LEN * BOOT_LINE_BUFS
CONFIG_BOOT_SERIAL_MAX_RECEIVE_SIZE=4096
```
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-newtmgr] bridadan commented on pull request #193: Allow setting the UART MTU from arguments to utilize larger serial pa…
Posted by "bridadan (via GitHub)" <gi...@apache.org>.
bridadan commented on PR #193:
URL: https://github.com/apache/mynewt-newtmgr/pull/193#issuecomment-1632778630
Any chance this could be merged? I'd love to make use of this speed up, thanks.
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-newtmgr] jakkra commented on pull request #193: Allow setting the UART MTU from arguments to utilize larger serial pa…
Posted by "jakkra (via GitHub)" <gi...@apache.org>.
jakkra commented on PR #193:
URL: https://github.com/apache/mynewt-newtmgr/pull/193#issuecomment-1641594542
@sjanc is it possible to release a new newtmgr with the few patches that was merged the tast few days? Many very useful things was added. And released to the https://archive.apache.org/dist/mynewt/ for download?
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-newtmgr] dopstar commented on a diff in pull request #193: Allow setting the UART MTU from arguments to utilize larger serial pa…
Posted by "dopstar (via GitHub)" <gi...@apache.org>.
dopstar commented on code in PR #193:
URL: https://github.com/apache/mynewt-newtmgr/pull/193#discussion_r1153890024
##########
nmxact/nmserial/serial_xport.go:
##########
@@ -287,7 +287,7 @@ func (sx *SerialXport) Tx(bytes []byte) error {
* carriage return (and possibly LF 2 bytes), */
/* all totaled, 124 bytes should work */
- writeLen := util.Min(124, totlen-written)
+ writeLen := util.Min(sx.cfg.Mtu - 4, totlen-written)
Review Comment:
the comment above should should say `MTU-4 bytes`
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow setting the UART MTU from arguments to utilize larger serial pa… [mynewt-newtmgr]
Posted by "davidtaylor-juul (via GitHub)" <gi...@apache.org>.
davidtaylor-juul commented on code in PR #193:
URL: https://github.com/apache/mynewt-newtmgr/pull/193#discussion_r1572844874
##########
nmxact/nmserial/serial_xport.go:
##########
@@ -50,7 +50,7 @@ var errTimeout error = errors.New("Timeout reading from serial connection")
func NewXportCfg() *XportCfg {
return &XportCfg{
ReadTimeout: 10 * time.Second,
- Mtu: 512,
+ Mtu: 128,
Review Comment:
Dropping a comment here for anyone else who may have found this was a breaking change in some cases.
This _does_ change the default value that is fed into `transceiver.go`, which does packet fragmentation at the layer above the serial transport (the CoAP layer). The value is read in the `MtuOut` method in [serial_sesn.go#L187](https://github.com/apache/mynewt-newtmgr/blob/cf9985f71c7fab6a71f345ab55438a7d58dc6a3f/nmxact/nmserial/serial_sesn.go#L187)
We had to set the MTU provided to `transceiver.go` to "unlimited" to prevent it from fragmenting the CoAP payload, and instead allow the `serial_xport.go` `TxCoap` method to fragment based on the actual MTU.
I'm not sure if there should be a broader fix for that, as it does seem redundant to have fragmentation at the CoAP layer, and again in the transport layer. At least over serial, fragmentation at the CoAP layer breaks re-assembly on the receiver end. Maybe other transports rely on the CoAP layer fragmenting and do not do transport layer fragmentation? I don't know, but in our use case it is necessary to disable the fragmentation in [transceiver.go#L294](https://github.com/apache/mynewt-newtmgr/blob/cf9985f71c7fab6a71f345ab55438a7d58dc6a3f/nmxact/mgmt/transceiver.go#L294) for serial connections.
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow setting the UART MTU from arguments to utilize larger serial pa… [mynewt-newtmgr]
Posted by "sjanc (via GitHub)" <gi...@apache.org>.
sjanc commented on code in PR #193:
URL: https://github.com/apache/mynewt-newtmgr/pull/193#discussion_r1574341781
##########
nmxact/nmserial/serial_xport.go:
##########
@@ -50,7 +50,7 @@ var errTimeout error = errors.New("Timeout reading from serial connection")
func NewXportCfg() *XportCfg {
return &XportCfg{
ReadTimeout: 10 * time.Second,
- Mtu: 512,
+ Mtu: 128,
Review Comment:
@davidtaylor-juul I'd suggest to send a PR that fix regression for you and we can follow up with discussion there
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-newtmgr] sjanc commented on a diff in pull request #193: Allow setting the UART MTU from arguments to utilize larger serial pa…
Posted by "sjanc (via GitHub)" <gi...@apache.org>.
sjanc commented on code in PR #193:
URL: https://github.com/apache/mynewt-newtmgr/pull/193#discussion_r1261541982
##########
nmxact/nmserial/serial_xport.go:
##########
@@ -50,7 +50,7 @@ var errTimeout error = errors.New("Timeout reading from serial connection")
func NewXportCfg() *XportCfg {
return &XportCfg{
ReadTimeout: 10 * time.Second,
- Mtu: 512,
+ Mtu: 128,
Review Comment:
this is new default value?
(commit message says default is not changed)
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-newtmgr] sjanc commented on pull request #193: Allow setting the UART MTU from arguments to utilize larger serial pa…
Posted by "sjanc (via GitHub)" <gi...@apache.org>.
sjanc commented on PR #193:
URL: https://github.com/apache/mynewt-newtmgr/pull/193#issuecomment-1641711681
Hi,
Yes, I plan to do Mynewt 1.11 release soon, hopefully rc1 will be out this month (it is way overdue anyway...)
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow setting the UART MTU from arguments to utilize larger serial pa… [mynewt-newtmgr]
Posted by "davidtaylor-juul (via GitHub)" <gi...@apache.org>.
davidtaylor-juul commented on code in PR #193:
URL: https://github.com/apache/mynewt-newtmgr/pull/193#discussion_r1572844874
##########
nmxact/nmserial/serial_xport.go:
##########
@@ -50,7 +50,7 @@ var errTimeout error = errors.New("Timeout reading from serial connection")
func NewXportCfg() *XportCfg {
return &XportCfg{
ReadTimeout: 10 * time.Second,
- Mtu: 512,
+ Mtu: 128,
Review Comment:
Dropping a comment here for anyone else who may have found this was a breaking change in some cases.
This _does_ change the default value that is fed into `transceiver.go`, which does packet fragmentation at the layer above the serial transport (the CoAP layer). The value is read in the `MtuOut` method in [serial_sesn.go#L187](https://github.com/apache/mynewt-newtmgr/blob/cf9985f71c7fab6a71f345ab55438a7d58dc6a3f/nmxact/nmserial/serial_sesn.go#L187)
We had to set the MTU provided to `transceiver.go` to "unlimited" to prevent it from fragmenting the CoAP payload, and instead allow the `serial_xport.go` `TxCoap` method to fragment based on the actual MTU.
I'm not sure if there should be a broader fix for that, as it does seem redundant to have fragmentation at the CoAP layer, and again in the transport layer. At least over serial, fragmentation at the CoAP layer breaks re-assembly on the receiver end. Maybe other transports rely on the CoAP layer fragmenting and do not do transport layer fragmentation? I don't know, but in our use case it is necessary to disable the fragmentation in [transceiver.go#L294](https://github.com/apache/mynewt-newtmgr/blob/cf9985f71c7fab6a71f345ab55438a7d58dc6a3f/nmxact/mgmt/transceiver.go#L294) for serial connections.
_edit: I should note that the CoAP layer fragmentation happening here is NOT regular CoAP TCP packetization, but is a blind binary split of the encoded CoAP payload_
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-newtmgr] sjanc merged pull request #193: Allow setting the UART MTU from arguments to utilize larger serial pa…
Posted by "sjanc (via GitHub)" <gi...@apache.org>.
sjanc merged PR #193:
URL: https://github.com/apache/mynewt-newtmgr/pull/193
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-newtmgr] jakkra commented on a diff in pull request #193: Allow setting the UART MTU from arguments to utilize larger serial pa…
Posted by "jakkra (via GitHub)" <gi...@apache.org>.
jakkra commented on code in PR #193:
URL: https://github.com/apache/mynewt-newtmgr/pull/193#discussion_r1261580922
##########
nmxact/nmserial/serial_xport.go:
##########
@@ -50,7 +50,7 @@ var errTimeout error = errors.New("Timeout reading from serial connection")
func NewXportCfg() *XportCfg {
return &XportCfg{
ReadTimeout: 10 * time.Second,
- Mtu: 512,
+ Mtu: 128,
Review Comment:
If you look at the other change you can see that this value was hardcoded before. This config.mtu was not even used. Hence the previous default (hardcoded value) was not changed.
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org