search

richardbrodie / fit-rs

A small crate used for reading and decoding FIT files generated by sports devices.
MIT License
19 stars 7 forks source link

Add tests #5

Open richardbrodie opened 5 years ago

Libbum commented 5 years ago

Coo!l I can parse Wahoo ELEMNT BOLT files now!

Heres one of my test files for the collection: https://cloud.exactlyinfinite.com/s/NBHrxtC2SiJtF2i

richardbrodie commented 5 years ago

Hmm, I'll take a look. What do you mean by the field number being different though?

I haven't given too much thought about exactly how the public API should work, so some suggestions from an actual user would be great. Obviously the philosophy of this crate is super-bare-bones so it's definitely quite user-unfriendly right now still.

I added a comment on #6 here: https://github.com/richardbrodie/fit-rs/issues/6#issuecomment-509633216

I have a few files from my bolt as well, and there are approximately 10 messages containing developer fields (actually a single field) in the whole file, so they're definitely not common.

I might not have been super clear, but the developer fields aren't included in the same Vec as the normal fields on a Message, rather normal fields are in Message.values and the new developer fields are in Message.dev_values.

Libbum commented 5 years ago

Yep, I followed the differences in the code between values and dev_values. BUT. I realise now that I was still filtering to just MessageType::Record. Without that, I do have a few, and they are generally DevDataField { data_index: 1, field_num: 0, value: U8(84) } with some form of u8 in there.

Field numbers being different: This is probably me reading the details in the README and confusing the output with the new mechanisms from the rework. My Record outputs look like this: [DataField { field_num: 253, value: Time(1562138822) }, DataField { field_num: 0, value: F32(59.356937) }, DataField { field_num: 1, value: F32(18.055645) }, DataField { field_num: 31, value: U8(2) }, DataField { field_num: 2, value: U16(2408) }, DataField { field_num: 9, value: I16(272) }, DataField { field_num: 5, value: U32(992645) }, DataField { field_num: 3, value: U8(172) }, DataField { field_num: 4, value: U8(66) }, DataField { field_num: 6, value: U16(5626) }, DataField { field_num: 13, value: I8(14) }]

I guess what I'm asking here is, is there a method I've overlooked to convert the message.values into the typed "Record" messages as seen in the readme? I can see that the build.rs calls the messages.semi.csv file to populate the field type enum for example: there, 253 is a timestamp. Now it seems that Message is the only public facing struct with these DataField values, no further parsing happens or can be accessed in the user space.

Is that analysis correct? If so then I can help working on turning this into something more public if that's a part of the goals of your crate.

Bare-bones: Totally fine, I'm quite happy doing the grunt work of processing what I need once the data is in a workable format. Of course we can just make the DataField members pub, then I can write my own match for example; but my question was more so checking if this was an expected outcome rather than something to work on further first.

richardbrodie commented 5 years ago

Ah, yeah, sorry about that. The readme is just old and inaccurate now. Since I rewrote things from scratch between 0.3 and 0.4 I threw out almost all conveniences.

My plan here was to be as bare-bones and, especially, performant as possible. So my thinking was that we remove absolutely everything that isn't necessary. Basically, the user would get something like a message_num 3 and field_num 253 and, with reference to the SDK, would know that that was a timestamp field.

It turns out that a MessageType enum was just as good as a message_num integer, but we haven't done the same for some kind of FieldType. I don't think it would impact performance too much to provide FieldType::Timestamp instead of 253, so would that be a substantially better state of affairs, or are you satisfied with me just making the DataField.field_num pub and letting you work with 253 instead?

On the subject of a not-yet-fixed-API, it's highly likely I will change fit::run to return some sort of struct like ParsedFit instead of a Vec<Message>. Not a huge change, I guess, since those Messages will be just one more method call away, but it makes it easier to return more state than just those.

Libbum commented 5 years ago

No worries, that all makes sense.

I think it's a better idea to have the crate as minimal as possible as you suggest. That way it's more likely that a larger set of devices files can be read without the necessity of supporting edge cases for everyone.

So keeping field_num as an int would be perfectly fine. The user would need to be able to parse a DataField though—making the members public would be the only change needed there.

There could of course be a wiki or a docs page later on when things settle down, giving additional parsing examples for individual devices that can be user submitted.

I'm travelling this week, but may have time to do something with this effort at some stage there. What would be the best aspect of things left that I could look into?

richardbrodie commented 5 years ago

I've pushed up a bumped version with pub fields for now, and I'll see what I can do to make it even more elegant.

I'm very happy to have some help, but just so you know I'm very disorganised so I'm not much use delegating ideas. However, if you feel like exploring what form a ParsedFit could take, and what members/methods it might need to have that would be pretty useful.

I guess something like:

pub struct ParsedFit {
  messages: Vec<Message>.
  developer_definitions: Vec<DeveloperFieldDescription>
}
impl ParsedFit {
  pub fn messages() -> Vec<Message>;
  pub fn dev_definitions() -> ...
  etc
}

With regards the SDK and still needing to refer to it when using this crate, I'm leaning towards splitting out the sdk module and the build.rs process as a separate crate where it would be more appropriate to add back some opt-in conveniences.