Hey Jef,
chasing a bug in Subsurface today I realized that we are abusing existing event types internally since we don't have reserved event types that we know will not be used by libdivecomputer in the future.
I guess there would be two solutions:
a) we ask you to add events that you haven't seen in any dive computer, yet, so we can use them as synthetic events for our own purposes (so far this is only a "setpoint change" event that we use in the planner).
b) you add a reserved range of numbers that we can simply redefine
SAMPLE_EVENT_APP_RESERVED_1 ... SAMPLE_EVENT_APP_RESERVED_8
(or some reasonable number) Then we can do a #define SAMPLE_EVENT_SUBSURFACE_SP_CHANGE SAMPLE_EVENT_APP_RESERVED_1 and all is well.
What do you think?
/D
On 2014-05-05 18:36, Dirk Hohndel wrote:
chasing a bug in Subsurface today I realized that we are abusing existing event types internally since we don't have reserved event types that we know will not be used by libdivecomputer in the future.
I guess there would be two solutions:
a) we ask you to add events that you haven't seen in any dive computer, yet, so we can use them as synthetic events for our own purposes (so far this is only a "setpoint change" event that we use in the planner).
b) you add a reserved range of numbers that we can simply redefine
SAMPLE_EVENT_APP_RESERVED_1 ... SAMPLE_EVENT_APP_RESERVED_8
(or some reasonable number) Then we can do a #define SAMPLE_EVENT_SUBSURFACE_SP_CHANGE SAMPLE_EVENT_APP_RESERVED_1 and all is well.
What do you think?
I'm not convinced this should be addressed with a change in libdivecomputer. The right solution would be to define your own set of events, and translate the libdivecomputer events to the corresponding subsurface events. Why should other applications have to deal with subsurface specific events? Imagine the mess we would end up with, if all other applications start to request their own events too.
Having said that, I think the easiest way to keep abusing the libdivecomputer events (e.g. a direct one-to-one mapping) and add some application defined events as well, is to set some high bit for your application specific events. Right now, the highest value is 25 (SAMPLE_EVENT_GASCHANGE2) and that value is very unlikely to ever grow past an 8 or 16 bits value.
Jef
On May 6, 2014, at 2:22 AM, Jef Driesen jef@libdivecomputer.org wrote:
On 2014-05-05 18:36, Dirk Hohndel wrote:
chasing a bug in Subsurface today I realized that we are abusing existing event types internally since we don't have reserved event types that we know will not be used by libdivecomputer in the future. I guess there would be two solutions: a) we ask you to add events that you haven't seen in any dive computer, yet, so we can use them as synthetic events for our own purposes (so far this is only a "setpoint change" event that we use in the planner). b) you add a reserved range of numbers that we can simply redefine SAMPLE_EVENT_APP_RESERVED_1 ... SAMPLE_EVENT_APP_RESERVED_8 (or some reasonable number) Then we can do a #define SAMPLE_EVENT_SUBSURFACE_SP_CHANGE SAMPLE_EVENT_APP_RESERVED_1 and all is well. What do you think?
I'm not convinced this should be addressed with a change in libdivecomputer. The right solution would be to define your own set of events, and translate the libdivecomputer events to the corresponding subsurface events. Why should other applications have to deal with subsurface specific events? Imagine the mess we would end up with, if all other applications start to request their own events too.
Which is why I suggested SAMPLE_EVENT_APP_RESERVED_xx - so any app can use those as needed.
Having said that, I think the easiest way to keep abusing the libdivecomputer events (e.g. a direct one-to-one mapping) and add some application defined events as well, is to set some high bit for your application specific events. Right now, the highest value is 25 (SAMPLE_EVENT_GASCHANGE2) and that value is very unlikely to ever grow past an 8 or 16 bits value.
And no one will ever need more than 640k RAM.
Maybe you are right - we need to rip out the libdivecomputer event model and create our own and simply populate it from the data libdivecomputer provides us. That way we can deal with some of the other annoyances that we have inherited from libdivecomputer as well
/D
On 2014-05-06 15:21, Dirk Hohndel wrote:
On May 6, 2014, at 2:22 AM, Jef Driesen jef@libdivecomputer.org wrote:
On 2014-05-05 18:36, Dirk Hohndel wrote:
chasing a bug in Subsurface today I realized that we are abusing existing event types internally since we don't have reserved event types that we know will not be used by libdivecomputer in the future. I guess there would be two solutions: a) we ask you to add events that you haven't seen in any dive computer, yet, so we can use them as synthetic events for our own purposes (so far this is only a "setpoint change" event that we use in the planner). b) you add a reserved range of numbers that we can simply redefine SAMPLE_EVENT_APP_RESERVED_1 ... SAMPLE_EVENT_APP_RESERVED_8 (or some reasonable number) Then we can do a #define SAMPLE_EVENT_SUBSURFACE_SP_CHANGE SAMPLE_EVENT_APP_RESERVED_1 and all is well. What do you think?
I'm not convinced this should be addressed with a change in libdivecomputer. The right solution would be to define your own set of events, and translate the libdivecomputer events to the corresponding subsurface events. Why should other applications have to deal with subsurface specific events? Imagine the mess we would end up with, if all other applications start to request their own events too.
Which is why I suggested SAMPLE_EVENT_APP_RESERVED_xx - so any app can use those as needed.
But then we introduce some unnecessary limit for libdivecomputer. Assume we set SAMPLE_EVENT_APP_RESERVED to 255. That's probably plenty of room for future events. But if we ever need more (unlikely, but who knows), then I can't add more. Unless I change SAMPLE_EVENT_APP_RESERVED, and break applications. So that's a bad solution for everyone.
Having said that, I think the easiest way to keep abusing the libdivecomputer events (e.g. a direct one-to-one mapping) and add some application defined events as well, is to set some high bit for your application specific events. Right now, the highest value is 25 (SAMPLE_EVENT_GASCHANGE2) and that value is very unlikely to ever grow past an 8 or 16 bits value.
And no one will ever need more than 640k RAM.
Let's say, that by the time we need all 32 bits (about 16M possible values) for the events, I probably lost my sanity already :-)
It's already a mess with just 25 events.
Maybe you are right - we need to rip out the libdivecomputer event model and create our own and simply populate it from the data libdivecomputer provides us. That way we can deal with some of the other annoyances that we have inherited from libdivecomputer as well
I think that's the only correct solution.
Jef
On Tue, 2014-05-06 at 16:00 +0200, Jef Driesen wrote:
b) you add a reserved range of numbers that we can simply redefine SAMPLE_EVENT_APP_RESERVED_1 ... SAMPLE_EVENT_APP_RESERVED_8 (or some reasonable number) Then we can do a #define SAMPLE_EVENT_SUBSURFACE_SP_CHANGE SAMPLE_EVENT_APP_RESERVED_1 and all is well. What do you think?
I'm not convinced this should be addressed with a change in libdivecomputer. The right solution would be to define your own set of events, and translate the libdivecomputer events to the corresponding subsurface events. Why should other applications have to deal with subsurface specific events? Imagine the mess we would end up with, if all other applications start to request their own events too.
Which is why I suggested SAMPLE_EVENT_APP_RESERVED_xx - so any app can use those as needed.
But then we introduce some unnecessary limit for libdivecomputer. Assume we set SAMPLE_EVENT_APP_RESERVED to 255. That's probably plenty of room for future events. But if we ever need more (unlikely, but who knows), then I can't add more. Unless I change SAMPLE_EVENT_APP_RESERVED, and break applications. So that's a bad solution for everyone.
I am obviously not making myself clear.
What I suggested was to have a handful of event numbers reserved. So after SAMPLE_EVENT_GASCHANGE2 you could add SAMPLE_EVENT_APP_RESERVED_1, SAMPLE_EVENT_APP_RESERVED_2,... and then if you need the next event for a new divecomputer you add it AFTER those values. That way it is future proof, no arbitrary maxima, etc.
YOUR proposal was that I should randomly assume some maximum value and use a high bit - THAT is what would possibly end up badly.
That is EXACTLY why I suggested NOT to do that and to have you reserve a few numbers that applications can use for their own purposes.
It's simple, it's obvious, everyone gets to use them which ever way they want, and no future change in libdivecomputer will ever break this.
Having said that, I think the easiest way to keep abusing the libdivecomputer events (e.g. a direct one-to-one mapping) and add some application defined events as well, is to set some high bit for your application specific events. Right now, the highest value is 25 (SAMPLE_EVENT_GASCHANGE2) and that value is very unlikely to ever grow past an 8 or 16 bits value.
And no one will ever need more than 640k RAM.
Let's say, that by the time we need all 32 bits (about 16M possible values) for the events, I probably lost my sanity already :-)
See above.
It's already a mess with just 25 events.
Maybe you are right - we need to rip out the libdivecomputer event model and create our own and simply populate it from the data libdivecomputer provides us. That way we can deal with some of the other annoyances that we have inherited from libdivecomputer as well
I think that's the only correct solution.
That is one of many correct solutions. And I guess it's the one I'll take.
/D
On 2014-05-06 16:12, Dirk Hohndel wrote:
On Tue, 2014-05-06 at 16:00 +0200, Jef Driesen wrote:
b) you add a reserved range of numbers that we can simply redefine SAMPLE_EVENT_APP_RESERVED_1 ... SAMPLE_EVENT_APP_RESERVED_8 (or some reasonable number) Then we can do a #define SAMPLE_EVENT_SUBSURFACE_SP_CHANGE SAMPLE_EVENT_APP_RESERVED_1 and all is well. What do you think?
I'm not convinced this should be addressed with a change in libdivecomputer. The right solution would be to define your own set of events, and translate the libdivecomputer events to the corresponding subsurface events. Why should other applications have to deal with subsurface specific events? Imagine the mess we would end up with, if all other applications start to request their own events too.
Which is why I suggested SAMPLE_EVENT_APP_RESERVED_xx - so any app can use those as needed.
But then we introduce some unnecessary limit for libdivecomputer. Assume we set SAMPLE_EVENT_APP_RESERVED to 255. That's probably plenty of room for future events. But if we ever need more (unlikely, but who knows), then I can't add more. Unless I change SAMPLE_EVENT_APP_RESERVED, and break applications. So that's a bad solution for everyone.
I am obviously not making myself clear.
What I suggested was to have a handful of event numbers reserved. So after SAMPLE_EVENT_GASCHANGE2 you could add SAMPLE_EVENT_APP_RESERVED_1, SAMPLE_EVENT_APP_RESERVED_2,... and then if you need the next event for a new divecomputer you add it AFTER those values. That way it is future proof, no arbitrary maxima, etc.
Sorry, I misunderstood. Regardless, my other concern remains valid. Why add some value that serves no purpose for libdivecomputer itself? Am I the only one that finds this a little odd?
YOUR proposal was that I should randomly assume some maximum value and use a high bit - THAT is what would possibly end up badly.
That is EXACTLY why I suggested NOT to do that and to have you reserve a few numbers that applications can use for their own purposes.
Well, my proposal was actually to properly map the libdivecomputer events to independent subsurface events. Setting the high bit was just a suggestion for a quick short-term solution.
It's simple, it's obvious, everyone gets to use them which ever way they want, and no future change in libdivecomputer will ever break this.
What if you need more than X reserved values in the future? Of course we can just add more reserved values to libdivecomputer. But then you are blocked until the next libdivecomputer release is available.
I'm still not convinced this is the best solution.
Maybe you are right - we need to rip out the libdivecomputer event model and create our own and simply populate it from the data libdivecomputer provides us. That way we can deal with some of the other annoyances that we have inherited from libdivecomputer as well
I think that's the only correct solution.
That is one of many correct solutions. And I guess it's the one I'll take.
I should have said "more correct" instead of "only" :-)
Jef
On Tue, May 6, 2014 at 6:21 AM, Dirk Hohndel dirk@hohndel.org wrote:
Maybe you are right - we need to rip out the libdivecomputer event model and create our own and simply populate it from the data libdivecomputer provides us. That way we can deal with some of the other annoyances that we have inherited from libdivecomputer as well
I'm the last to think that the libdivecomputer event model is good, but I think translating it to some saner internal format is likely even worse. Because then you have the "new version of libdivecomputer, new event number, now we need to add more translation, and in the meantime we lose data".
So I'd suggest against translating the event numbers. Add a bit to our structure saying "this is our private event", and just make that be the "exclusive" range.
Linus
On 06-05-14 18:13, Linus Torvalds wrote:
On Tue, May 6, 2014 at 6:21 AM, Dirk Hohndel dirk@hohndel.org wrote:
Maybe you are right - we need to rip out the libdivecomputer event model and create our own and simply populate it from the data libdivecomputer provides us. That way we can deal with some of the other annoyances that we have inherited from libdivecomputer as well
I'm the last to think that the libdivecomputer event model is good, but I think translating it to some saner internal format is likely even worse. Because then you have the "new version of libdivecomputer, new event number, now we need to add more translation, and in the meantime we lose data".
How is that different from what we have today? If libdivecomputer gets some new event today, then subsurface will not be able to use it, until some code is added to handle it properly. How else will it know the interpretation for the new value? Without the correct interpretation, it's just some meaningless number.
I don't see much difference between adding an extra translation or some extra code to handle the new event directly.
(The exception here is when you just store the new number as-is, and then an more recent subsurface version, which does know about the new event, will be able to interpret it correctly afterwards. But, correct me if I'm wrong, I don't think subsurface does that right now. Subsurface stores the event name in the xml and not the event number, if I remember correctly?)
Jef
On Tue, May 6, 2014 at 10:44 AM, Jef Driesen jef@libdivecomputer.org wrote:
How is that different from what we have today? If libdivecomputer gets some new event today, then subsurface will not be able to use it, until some code is added to handle it properly.
Sure it can. We may not know what it is, but we can still save the event number and data, and at least it's there. And this is not theoretical. We've done this.
If we use some totally disjoint "these are our events", we can't do that.
This is the *only* reason we use that horrid type/flags/value model in the first place. But it's a big reason. And you just seemed to totally dismiss it.
Linus
On 2014-05-06 19:57, Linus Torvalds wrote:
On Tue, May 6, 2014 at 10:44 AM, Jef Driesen jef@libdivecomputer.org wrote:
How is that different from what we have today? If libdivecomputer gets some new event today, then subsurface will not be able to use it, until some code is added to handle it properly.
Sure it can. We may not know what it is, but we can still save the event number and data, and at least it's there. And this is not theoretical. We've done this.
If we use some totally disjoint "these are our events", we can't do that.
This is the *only* reason we use that horrid type/flags/value model in the first place. But it's a big reason. And you just seemed to totally dismiss it.
I didn't realize Subsurface stored the event data as-is. I just looked at the handle_event function where the event type gets translated into the corresponding event name. So that's why I assumed subsurface does already map the event to something else. I just didn't notice you also keep the event type around (and save it to the xml too). So yes, you are right on that. My apologies.
Anyway, I think your suggestions of adding an extra bit to distinguish between a libdivecomputer and a subsurface specific event is a better solution than introducing some reserved value(s) in libdivecomputer. Then you can have as many internal events as you need, without needing a dependency on libdivecomputer.
Jef
-
Dirk Hohndel -
Jef Driesen -
Linus Torvalds