-
Notifications
You must be signed in to change notification settings - Fork 362
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
sys/log: Add number of entries support in log #3168
base: master
Are you sure you want to change the base?
Conversation
Would be good to explain what's the purpose of logging number of entries in a log itself. Since you introduce new log version which is not backwards compatible, why no just revamp this so we don't need to add new log version and bunch of ifdefs every time a new field is added, e.g. by using tlv. |
The purpose of logging number of entries in the entry itself is that we do not have to walk all the entries to count the number of entries. For a large number of log entries, walking logs serially to find the end counting number of entries is little slow. So, this gives us a way to persist number of entries and retrieve number of entries whenever needed without having to walk the log. This change is supposed to be backwards compatible since the base header is the same 15 bytes but somehow entries with log version 3 are not getting read correctly. I am still figuring out why that is happening. We wanted to open a PR with changes sooner than later and hence I thought the backwards compatibility fix could be added later on in a follow on PR. It does not require any format/log header changes as such. The ifdefs can actually be removed to be similar to image hash. The only reason I kept them is to save us some stack space and code space. TLV would be a good idea but wouldn't that require too many changes to the logging code ? I wanted to keep the change backwards compatible. |
If we want to make this change such that it can log various entries of different types, flags could be utilized but this header in itself won't be enough to make the log format generic enough, perhaps we could agree upon a log header/trailer format and define its functional aspect. I am open to making some changes to the way it works but do keep in mind we want to keep this change backwards compatible with log version 3. |
If the base header is the same, you don't need the log version 4 in the first place. The new struct for v4 is redundant and also invalid since counter doesn't need to follow img hash - this depends on flags. |
Yes, that’s what I have been thinking as well. I think the log version 3 was added at the time to add support for flags. So, I should be able to just add a field to the same header. Haven’t gotten to this yet again. Will let you know after I try it again and will update this PR accordingly. |
@andrzej-kaczmarek I did try this out, keeping the same log version works fine with upgrades but downgrades do not work as the header now has an extra field which the old firmware does not know about which is fine in a way. I am thinking of adding the num_entries field after the log entry body, that way downgrades will work fine as well. Another option which we spoke about internally was to read the logs at bootup and populate number of entries and keep incrementing in RAM which is doable but then there are corner cases to worry about. What are your thoughts on it ? |
Please check email on the dev list for more information:
LOG_FLAGS_NUM_ENTRIES
CLI command:
Example log entries: