[GitHub] ccollins476ad commented on issue #1173: Awkward log API requires padded buffer
ccollins476ad commented on issue #1173: Awkward log API requires padded buffer URL: https://github.com/apache/mynewt-core/issues/1173#issuecomment-397086367 Andrzej, that sounds good, though it might be a bit tricky. Perhaps it can be addressed in a later PR :). This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ccollins476ad commented on issue #1173: Awkward log API requires padded buffer
ccollins476ad commented on issue #1173: Awkward log API requires padded buffer URL: https://github.com/apache/mynewt-core/issues/1173#issuecomment-397047321 I thought of an obvious solution to this problem: 1. The `log_append` functions define a small buffer on the stack, with size equal to `sizeof (struct log_entry_hdr)` rounded up to the nearest min-write-size multiple. 2. The `log_append` functions split their write into two phases: a. header + first-x bytes of the body b. remaining bytes in the message body (where "first-x" is however many bytes are required to increase the chunk size up to a multiple of the flash min-write-size) This doesn't address the mbuf issue, but I don't think there is any easy way around that. I think the right solution in this case is to require a min-write-size of 1 for the `append_mbuf` API. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ccollins476ad commented on issue #1173: Awkward log API requires padded buffer
ccollins476ad commented on issue #1173: Awkward log API requires padded buffer URL: https://github.com/apache/mynewt-core/issues/1173#issuecomment-397047321 I thought of an obvious solution to this problem: 1. The `log_append` functions define a small buffer on the stack, with size equal to `sizeof (struct log_entry_hdr)` rounded up to the nearest min-write-size multiple. 2. The `log_append` functions split their write into two phases: a. header + first-x bytes of the body b. remaining bytes in the message body This doesn't address the mbuf issue, but I don't think there is any easy way around that. I think the right solution in this case is to require a min-write-size of 1 for the `append_mbuf` API. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ccollins476ad commented on issue #1173: Awkward log API requires padded buffer
ccollins476ad commented on issue #1173: Awkward log API requires padded buffer URL: https://github.com/apache/mynewt-core/issues/1173#issuecomment-396791926 Looking into this a bit more, I discovered a few things: 1. The `log_append_mbuf` and `log_append_mbuf_typed` functions assume a flash write size of 1. So these won't work on some hardware. 2. A lot of our system relies on the fact that the header and body are adjacent in a log entry. So my idea of inserting padding as needed after the entry header would cause quite a headache. Re: 2- Specifically, the `log_read()` function fills a buffer with data from the log, starting with the log entry header. The caller then skips past the header in the buffer to access the message body. This is problematic if there might be padding after the header. This issue also affects log walks; a typical walk callback reads the current entry using `log_read()`. We could add a "header-size" callback to `struct log_handler`. Using this, the caller could know where the log body begins. Alternatively, the `log_read()` function could return two pieces of data: 1. The entry header (`struct log_entry_hdr`) 2. Part of the message body (length equal to number of bytes requested) So, the caller would specify the number of body bytes to read, and would always get back the full header and the specified number of body bytes. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ccollins476ad commented on issue #1173: Awkward log API requires padded buffer
ccollins476ad commented on issue #1173: Awkward log API requires padded buffer URL: https://github.com/apache/mynewt-core/issues/1173#issuecomment-396791926 Looking into this a bit more, I discovered a few things: 1. The `log_append_mbuf` and `log_append_mbuf_typed` functions assume a flash write size of 1. So these won't work on some hardware. 2. A lot of our system relies on the fact that the header and body are adjacent in a log entry. So my idea of inserting padding as needed after the entry header would cause quite a headache. Re: 2- Specifically, the `log_read()` function fills a buffer with data from the log, starting with the log entry header. The caller then skips past the header in the buffer to access the message body. This is problematic if there might be padding after the header. This issue also affects log walks; a typical walk callback reads the current entry using `log_read()`. We could add a "header-size" callback to `struct log_handler`. Using this, the called could know where the log body begins. Alternatively, the `log_read()` function could return two pieces of data: 1. The entry header (`struct log_entry_hdr`) 2. Part of the message body (length equal to number of bytes requested) So, the caller would specify the number of body bytes to read, and would always get back the full header and the specified number of body bytes. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ccollins476ad commented on issue #1173: Awkward log API requires padded buffer
ccollins476ad commented on issue #1173: Awkward log API requires padded buffer
URL: https://github.com/apache/mynewt-core/issues/1173#issuecomment-396735662
Thanks for the correction, Marko. That makes sense.
I propose the following changes:
1. Modify the log entry header such that the `ue_level` field is split into
two fields: "level" and "flags" (one nibble each).
2. Define a `LOG_ENTRY_HDR_F_PADDED` flag.
3. If this new flag is set in an entry, it indicates that the log entry body
begins on the next flash-aligned address, rather than immediately after the log
header.
4. Change the semantics of the `log_append{,_typed,_mbuf}` functions such
that the provided payload does not contain room for a header. Instead, these
function define the entry header on the stack, fill it, and write it separately.
Notes:
* The over-the-air format of log commands and responses would not change.
* I chose to split the `ue_level` field because I think 16 levels is plenty.
I also don't think it makes too much sense to allow an application to define
custom "per-user" levels, since levels are necessarily monotonically
increasing. I.e., all custom levels would be more severe than "critical".
To maintain backwards compatibility, we would need to either:
1. Create newly named append functions with the new semantics, or
2. Introduce a new log version or new syscfg setting to enable the new
behavior.
Alternative solution:
Define a new log version (4). In this version, entry headers are always
padded.
The second solution is much easier to implement, but makes it more difficult
to upgrade devices with existing data in their logs.
Thoughts?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ccollins476ad commented on issue #1173: Awkward log API requires padded buffer
ccollins476ad commented on issue #1173: Awkward log API requires padded buffer
URL: https://github.com/apache/mynewt-core/issues/1173#issuecomment-396735662
Thanks for the correction, Marko. That makes sense.
I propose the following changes:
1. Modify the log entry header such that the `ue_level` field is split into
two fields: "level" and "flags" (one nibble each).
2. Define a `LOG_ENTRY_HDR_F_PADDED` flag.
3. If this new flag is set in an entry, it indicates that the log entry body
begins on the next flash-aligned address, rather than immediately after the log
header.
4. Change the semantics of the `log_append{,_typed,_mbuf}` functions such
that the provided payload does not contain room for a header. Instead, these
function define the entry header on the stack, fill it, and write it separately.
Notes:
* The over-the-air format of log commands and responses would not change.
* I chose to split the `ue_level` field because I think 16 levels is plenty.
I also don't think it makes too much sense to allow an application to define
custom "per-user" levels, since levels are necessarily monotonically
increasing. I.e., all custom levels would be more severe than "critical".
To maintain backwards compatibility, we would need to either:
1. Create newly named append functions with the new semantics, or
2. Introduce a new log version or new syscfg setting to enable the new
behavior.
Thoughts?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ccollins476ad commented on issue #1173: Awkward log API requires padded buffer
ccollins476ad commented on issue #1173: Awkward log API requires padded buffer
URL: https://github.com/apache/mynewt-core/issues/1173#issuecomment-396735662
Thanks for the correction, Marko. That makes sense.
I propose the following changes:
1. Modify the log entry header such that the `ue_level` field is split into
two fields: "level" and "flags" (one nibble each).
2. Define a `LOG_ENTRY_HDR_F_PADDED` flag.
3. If this new flag is set in an entry, it indicates that the log entry body
begins on the next flash-aligned address, rather than immediately after the log
header.
4. Change the semantics of the `log_append{,_typed,_mbuf}` functions such
that the provided payload does not contain room for a header. Instead, these
function define the entry header on the stack, fill it, and write it separately.
Notes:
* The over-the-air format of log commands and responses would not change.
* I chose to split the `ue_level` field because I think 16 levels is plenty.
I also don't think it makes too much sense to allow an application to define
custom "per-user" levels, since levels be monotonically increasing. I.e., all
custom levels would be more severe than "critical".
To maintain backwards compatibility, we would need to either:
1. Create newly named append functions with the new semantics, or
2. Introduce a new log version or new syscfg setting to enable the new
behavior.
Thoughts?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ccollins476ad commented on issue #1173: Awkward log API requires padded buffer
ccollins476ad commented on issue #1173: Awkward log API requires padded buffer
URL: https://github.com/apache/mynewt-core/issues/1173#issuecomment-396735662
Thanks for the correction, Marko. That makes sense.
I propose the following changes:
1. Modify the log entry header such that the `ue_level` field is split into
two fields: "level" and "flags" (one nibble each).
2. Define a `LOG_ENTRY_HDR_F_PADDED` flag.
3. If this new flag is set in an entry, it indicates that the log entry body
begins on the next flash-aligned address, rather than immediately after the log
header.
4. Change the semantics of the `log_append{,_typed,_mbuf}` functions such
that the provided payload does not contain room for a header. Instead, these
function define the entry header on the stack, fill it, and write it separately.
Notes:
* The over-the-air format of log commands and responses would not change.
* I chose to split the `ue_level` field because I don't think 16 levels is
plenty. I also don't think it makes too much sense to allow an application to
define custom "per-user" levels, since levels be monotonically increasing.
I.e., all custom levels would be more severe than "critical".
To maintain backwards compatibility, we would need to either:
1. Create newly named append functions with the new semantics, or
2. Introduce a new log version or new syscfg setting to enable the new
behavior.
Thoughts?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
