Hi,
I have been working on a new api for gas changes. The idea is that old gas change events (SAMPLE_EVENT_GASCHANGE and SAMPLE_EVENT_GASCHANGE2) will be replaced with a new DC_SAMPLE_GASMIX sample. The corresponding sample data is an integer with the index of the gas mix. The actual oxygen and helium percentages can be retrieved with the DC_FIELD_GASMIX api. The main advantage over the old events, is that the gas mix index allows to distinguish two identical gas mixes.
I have attached two patches. The first patch contains the new api, and with the second patch all backends are ported to the new api.
Please note that applying these patches will break existing applications. They will still build, but they will no longer get any gas change events. In order to maintain backwards compatibility, I'm planning to introduce the new api as follows: The old events will get marked as deprecated, but not removed yet. That ensures existing applications will continue to work with no changes. In the meantime, applications can start to use the new api. And once v0.5 is out, the old events will be removed.
Comments are welcome!
PS: I'm leaving tonight for a diving trip, so I probably won't be able to respond to questions or comments during the next week.
Jef
On Fri, May 22, 2015 at 04:34:45PM +0200, Jef Driesen wrote:
I have been working on a new api for gas changes. The idea is that old gas change events (SAMPLE_EVENT_GASCHANGE and SAMPLE_EVENT_GASCHANGE2) will be replaced with a new DC_SAMPLE_GASMIX sample. The corresponding sample data is an integer with the index of the gas mix. The actual oxygen and helium percentages can be retrieved with the DC_FIELD_GASMIX api. The main advantage over the old events, is that the gas mix index allows to distinguish two identical gas mixes.
Yes, we'd love to see that.
I have attached two patches. The first patch contains the new api, and with the second patch all backends are ported to the new api.
Comments on the patches below.
Please note that applying these patches will break existing applications.
Life could be so much easier if you did the one simple thing that Linus and I have suggested quite a few time which would allow applications to detect which features are in the version of libdivecomputer that they are building against and which aren't. Your vision of doing this has always been in the context of "releases", assuming that the consumers of libdivecomputer won't be building against master - but that just isn't how many people appear to be using libdivecomputer. And because all the constants are enums and not defines, there is no easy way to test what's there and what isn't.
In the Subsurface branch we have trivial things like this:
diff --git a/include/libdivecomputer/parser.h b/include/libdivecomputer/parser.h index 774e5e3242bb..dbd9d2d81cf5 100644 --- a/include/libdivecomputer/parser.h +++ b/include/libdivecomputer/parser.h @@ -59,9 +59,13 @@ typedef enum dc_field_type_t { DC_FIELD_TEMPERATURE_MAXIMUM, DC_FIELD_TANK_COUNT, DC_FIELD_TANK, - DC_FIELD_DIVEMODE + DC_FIELD_DIVEMODE, + DC_FIELD_STRING, } dc_field_type_t;
+// Make it easy to test support compile-time with "#ifdef DC_FIELD_STRING" +#define DC_FIELD_STRING DC_FIELD_STRING +
Redundant? Ugly? You tell me. But most importantly exceptionally useful. Because code that requires DC_FIELD_STRING can now simply do
#ifdef DC_FIELD_STRING // ah, we have that feature, let's use it... ... #endif
They will still build, but they will no longer get any gas change events. In order to maintain backwards compatibility, I'm planning to introduce the new api as follows: The old events will get marked as deprecated, but not removed yet. That ensures existing applications will continue to work with no changes. In the meantime, applications can start to use the new api. And once v0.5 is out, the old events will be removed.
But that's not what the patches below do, is it? They just remove support for the old event from the backends.
I'd rather have you do what you describe here - support both for a while, but make it possible to detect if the new SAMPLE type is available (which then allows the app to ignore the deprecated events).
PS: I'm leaving tonight for a diving trip, so I probably won't be able to respond to questions or comments during the next week.
We all feel very sad for your pain and suffering...
diff --git a/include/libdivecomputer/parser.h b/include/libdivecomputer/parser.h index 774e5e3..c22d26e 100644 --- a/include/libdivecomputer/parser.h +++ b/include/libdivecomputer/parser.h @@ -43,7 +43,8 @@ typedef enum dc_sample_type_t { DC_SAMPLE_SETPOINT, DC_SAMPLE_PPO2, DC_SAMPLE_CNS,
- DC_SAMPLE_DECO
- DC_SAMPLE_DECO,
- DC_SAMPLE_GASMIX
} dc_sample_type_t;
Yeah, I'd REALLY want a
#define DC_SAMPLE_GASMIX DC_SAMPLE_GASMIX
here. Because then the application could KNOW that the new sample type is available.
diff --git a/src/atomics_cobalt_parser.c b/src/atomics_cobalt_parser.c index 83bcfc7..925e05a 100644 --- a/src/atomics_cobalt_parser.c +++ b/src/atomics_cobalt_parser.c @@ -306,13 +306,8 @@ atomics_cobalt_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback ERROR (abstract->context, "Invalid gas mix index."); return DC_STATUS_DATAFORMAT; }
unsigned int o2 = data[SZ_HEADER + SZ_GASMIX * idx + 4];unsigned int he = data[SZ_HEADER + SZ_GASMIX * idx + 5];sample.event.type = SAMPLE_EVENT_GASCHANGE2;sample.event.time = 0;sample.event.flags = 0;sample.event.value = o2 | (he << 16);if (callback) callback (DC_SAMPLE_EVENT, sample, userdata);
sample.gasmix = idx;if (callback) callback (DC_SAMPLE_GASMIX, sample, userdata); gasmix_previous = gasmix;
So you are no longer sending the event.
I would much rather see you send the event AND provide the DC_SAMPLE_GASMIX. This way (assuming you took my previous suggestion), the application can simply say "if DC_SAMPLE_GASMIX isn't defined, respond to the two GASCHANGE events, but if it is defined then ignore the GASCHANGE events and instead use the DC_SAMPLE_GASMIX".
Now old software works with the new library. And updated software works with both the old and the new library.
Ta-Da!
/D
On 2015-05-22 17:49, Dirk Hohndel wrote:
Life could be so much easier if you did the one simple thing that Linus and I have suggested quite a few time which would allow applications to detect which features are in the version of libdivecomputer that they are building against and which aren't. Your vision of doing this has always been in the context of "releases", assuming that the consumers of libdivecomputer won't be building against master - but that just isn't how many people appear to be using libdivecomputer. And because all the constants are enums and not defines, there is no easy way to test what's there and what isn't.
In most software projects, you are supposed to use "releases", and not development versions. Unless you are developer and want to try the bleeding edge of course. Libdivecomputer is no different in that regard.
While I understand your arguments, introducing a detection macro for every single feature that gets added or changed during the *development* cycle, also means I have to maintain those forever, even after the release. [As a side note: what's the point of doing releases if you're always going to rely on feature detection macros?] But they are not a perfect solution either. For example with the DC_FIELD_TANK api, the interpretation of the volume field of the dc_tank_t struct has been changed in an incompatible way. You won't be able to catch such changes with the presence of a simple DC_FIELD_TANK macro.
I guess the relevant question here is: why is everyone using the master branch right now? The answer is probably because the next release has been delayed several times already now. The previous release was more than a year ago, so you basically need master to take advantage of some new features that should have been released long ago. I actually intended to do shorter release cycles (e.g. every 3-4 months). Unfortunately some of the features that I wanted in v0.5 are taking me much longer than expected. I'm not really satisfied with this situation either!
Now, let's assume for a moment that libdivecomputer would have more frequent releases. Would you still be using the master branch for subsurface builds? For the stable branches, the DC_VERSION_CHECK macro should do the trick just fine. And when building against master (in order to try the latest stuff, and not some feature that should have been released long ago, like is the case now), I don't think it's unreasonable to require the latest version. Right?
How about doing the long awaited v0.5 release now? It won't have all the features that I originally planned. But instead of waiting until everything is finally ready, I just make a few (smaller) intermediate releases.
They will still build, but they will no longer get any gas change events. In order to maintain backwards compatibility, I'm planning to introduce the new api as follows: The old events will get marked as deprecated, but not removed yet. That ensures existing applications will continue to work with no changes. In the meantime, applications can start to use the new api. And once v0.5 is out, the old events will be removed.
But that's not what the patches below do, is it? They just remove support for the old event from the backends.
I'd rather have you do what you describe here - support both for a while, but make it possible to detect if the new SAMPLE type is available (which then allows the app to ignore the deprecated events).
I started working on these patches several months ago. At that time, the plan was still to introduce them after the v0.5 release. That's why the attached patches do indeed remove support for the old events. But don't worry, I'll make sure that the old events are restored before applying them. Sorry, for the confusion. I should have mentioned that more explicitly.
Jef
On Wed, Jun 03, 2015 at 04:16:09PM +0200, Jef Driesen wrote:
On 2015-05-22 17:49, Dirk Hohndel wrote:
Life could be so much easier if you did the one simple thing that Linus and I have suggested quite a few time which would allow applications to detect which features are in the version of libdivecomputer that they are building against and which aren't. Your vision of doing this has always been in the context of "releases", assuming that the consumers of libdivecomputer won't be building against master - but that just isn't how many people appear to be using libdivecomputer. And because all the constants are enums and not defines, there is no easy way to test what's there and what isn't.
In most software projects, you are supposed to use "releases", and not development versions. Unless you are developer and want to try the bleeding edge of course. Libdivecomputer is no different in that regard.
If one uses the release versions only, one usually misses out on a lot of the most recent fixes.
While I understand your arguments, introducing a detection macro for every single feature that gets added or changed during the *development* cycle, also means I have to maintain those forever, even after the release. [As a side note: what's the point of doing releases if you're always going to rely on feature detection macros?] But they are not a perfect solution either. For example with the DC_FIELD_TANK api, the interpretation of the volume field of the dc_tank_t struct has been changed in an incompatible way. You won't be able to catch such changes with the presence of a simple DC_FIELD_TANK macro.
I guess the relevant question here is: why is everyone using the master branch right now? The answer is probably because the next release has been delayed several times already now. The previous release was more than a year ago, so you basically need master to take advantage of some new features that should have been released long ago.
Yep
I actually intended to do shorter release cycles (e.g. every 3-4 months). Unfortunately some of the features that I wanted in v0.5 are taking me much longer than expected. I'm not really satisfied with this situation either!
And what I'm telling you is that releases really don't matter for me that much. I'm now bundling libdivecomputer with Subsurface and that means I don't assume that people will use Subsurface together with a random release of libdivecomputer.
Now, let's assume for a moment that libdivecomputer would have more frequent releases. Would you still be using the master branch for subsurface builds?
Almost certainly.
For the stable branches, the DC_VERSION_CHECK macro should do the trick just fine. And when building against master (in order to try the latest stuff, and not some feature that should have been released long ago, like is the case now), I don't think it's unreasonable to require the latest version. Right?
Yes it is. That's why I'm bundling it now. People want to run Subsurface on ancient distributions. We still support Ubuntu 12.04 for example (that will be dropped in 4.5, though).
How about doing the long awaited v0.5 release now? It won't have all the features that I originally planned. But instead of waiting until everything is finally ready, I just make a few (smaller) intermediate releases.
That's entirely up to you. At this point my plan is to continue to base the Subsurface-testing and Subsurface-x.y branches on master. And as I am bundling them with a Subsurface release all I need to make sure is that they match.
For the Subsurface developers it means that they need to track my branches, but I think that's what most of them do, anyway.
/D
On Wed, Jun 3, 2015 at 7:16 AM, Jef Driesen jef@libdivecomputer.org wrote:
In most software projects, you are supposed to use "releases", and not development versions. Unless you are developer and want to try the bleeding edge of course. Libdivecomputer is no different in that regard.
Jef, your releases haven't been timely enough to use, and they aren't any simpler to distinguish _either_.
If you were to make releases much more often, and if those releases had clear ways to test for features, your argument would be stronger. As ti is, people pretty much *have* to use non-release versions.
Your last release was what, 15 months ago?
So don't tell people to use releases, when you then make it a non-option. If you expect people to primarily use released versions, you need to release them _much_ more often. Several times a year, not "every year or two".
I think one of the issues is that you have these "big features" you want to get done, and that is always a problem for release timing. I used to do that with the kernel too (_years_ between 2.4 and 2.6 stable releases), but even back then I did "developer releases" much more often, and would encourage people to use them.
The whole "make a release every two or three months, whether things have changed a lot or not" makes things much easier both as a developer (no "big plans" to worry about, no lack of testing in between big changes etc) and as a user (no huge jumps). I can heartily recommend it.
But even then, it's actually much better to make the "does this version support XYZ" depend on something like
#ifdef XYZ
than on
#if version_newer_than(5,1,1)
because it makes it *much* easier to both back-port features to stable sub-releases (eg think of a situation where you added something to 0.5, but then also backported it to 0.4.2, so now that "version_newer_than()" thing gets to be a mess), and makes the use much more obvious.
It also makes it much easier to _test_ new features - stuff that isn't necessarily ready for a release, but that you want testing for.
And there's very much a chicken-and-egg problem: how do you intend to make a good release with a solid new feature, if you make it hard for users to test that feature?
So please use that feature-define model rather than the _completely_ broken "release numbering test" model. Because really, it *is* completely broken.
Linus
On 03-06-15 16:54, Linus Torvalds wrote:
On Wed, Jun 3, 2015 at 7:16 AM, Jef Driesen jef@libdivecomputer.org wrote:
In most software projects, you are supposed to use "releases", and not development versions. Unless you are developer and want to try the bleeding edge of course. Libdivecomputer is no different in that regard.
Jef, your releases haven't been timely enough to use, and they aren't any simpler to distinguish _either_.
I assume you are referring to the features in a release, and not the version itself? Because that's easy to check with DC_VERSION_CHECK and friends.
If you were to make releases much more often, and if those releases had clear ways to test for features, your argument would be stronger. As ti is, people pretty much *have* to use non-release versions.
Your last release was what, 15 months ago?
So don't tell people to use releases, when you then make it a non-option. If you expect people to primarily use released versions, you need to release them _much_ more often. Several times a year, not "every year or two".
I think one of the issues is that you have these "big features" you want to get done, and that is always a problem for release timing. I used to do that with the kernel too (_years_ between 2.4 and 2.6 stable releases), but even back then I did "developer releases" much more often, and would encourage people to use them.
The whole "make a release every two or three months, whether things have changed a lot or not" makes things much easier both as a developer (no "big plans" to worry about, no lack of testing in between big changes etc) and as a user (no huge jumps). I can heartily recommend it.
Yeah, I know it's been *way* too long. I intended to do shorter release cycles. But because I wanted to introduce some major (backwards incompatible) changes after the next v0.5 release, I also wanted to finish a few other things first. But that todo list only grew, with the known result...
So it looks like it doesn't work really well for me either. So let's try a more time based release cycle.
But even then, it's actually much better to make the "does this version support XYZ" depend on something like
#ifdef XYZthan on
#if version_newer_than(5,1,1)because it makes it *much* easier to both back-port features to stable sub-releases (eg think of a situation where you added something to 0.5, but then also backported it to 0.4.2, so now that "version_newer_than()" thing gets to be a mess), and makes the use much more obvious.
In the stable releases, I'm only doing pure bugfixing. No new features get backported there, so that shouldn't be a problem.
It also makes it much easier to _test_ new features - stuff that isn't necessarily ready for a release, but that you want testing for.
And there's very much a chicken-and-egg problem: how do you intend to make a good release with a solid new feature, if you make it hard for users to test that feature?
That's indeed a very good argument.
Anyway, assume I provide feature macros from now on, my main concerns are as follows:
I consider the master branch as work-in-progress. This is where all new features get introduced. But since it's a new feature, it's not necessary right from the start. So it might take a few iterations to get the api right. With a feature detection macro, you'll be able to detect the presence of the feature itself, but not each intermediate variant. That means an application may no longer work correctly or even build against a certain range of commits on master. Do you consider that a problem? (Of course I'll try to avoid those kind of problems, but for me the point of a development branch is being able to change things without having to worry too much about possibly breaking stuff.)
A feature macro only works at build time, and for C/C++ applications. But there are several non C/C++ applications (written in .NET, python, maybe even some java ones) out there that can't use macros at all. Their only option is runtime version detection using the dc_version_check() function. Since those applications are certainly not second class citizens, the version based detection will always remain the reference. But in addition there will be feature detection macros.
Does that sounds a reasonable approach?
So please use that feature-define model rather than the _completely_ broken "release numbering test" model. Because really, it *is* completely broken.
The release number model is nevertheless used by many (most?) software libraries. But let's not argue about that any further :-)
Jef
On Fri, May 22, 2015 at 7:34 AM, Jef Driesen jef@libdivecomputer.org wrote:
I have been working on a new api for gas changes. The idea is that old gas change events (SAMPLE_EVENT_GASCHANGE and SAMPLE_EVENT_GASCHANGE2) will be replaced with a new DC_SAMPLE_GASMIX sample. The corresponding sample data is an integer with the index of the gas mix.
Side note: did you consider just doing both gasmix and index? Document that either (or both) will be set, and make "index=-1" be the "I don't have an index".
We end up doing that in subsurface anyway. Now, it's mostly historical: because we had the gasmix, that's what we want in some places, but then we _also_ want the index for disambiguation.
But it turns out having both actually has some advantages, even though it sounds redundant.
For example, on some dive computers, you might not *get* an index (because the dive computer fundamentally just reports O2 percentage in the event). So then it would be better to say "I don't know the index" than basically make up a possibly ambiguous cylinder.
And I think some others (Petrel?) allow you to change the mix during the dive for a special cylinder (perhaps you set your mixes wrong, or perhaps you're breathing a buddy gas). Sure, you can give the index for the special cylinder, but what happens if the user changed the gasmix twice? I'm not sure what the dive computer reports, but from a abstraction standpoint it would be good to give both index and mix - at least there's no dropped information, even if it might also confuse some user of libdivecomputer.
Linus
On 03-06-15 17:41, Linus Torvalds wrote:
On Fri, May 22, 2015 at 7:34 AM, Jef Driesen jef@libdivecomputer.org wrote:
I have been working on a new api for gas changes. The idea is that old gas change events (SAMPLE_EVENT_GASCHANGE and SAMPLE_EVENT_GASCHANGE2) will be replaced with a new DC_SAMPLE_GASMIX sample. The corresponding sample data is an integer with the index of the gas mix.
Side note: did you consider just doing both gasmix and index? Document that either (or both) will be set, and make "index=-1" be the "I don't have an index".
We end up doing that in subsurface anyway. Now, it's mostly historical: because we had the gasmix, that's what we want in some places, but then we _also_ want the index for disambiguation.
But it turns out having both actually has some advantages, even though it sounds redundant.
No, I didn't consider this. I think it will only complicates the api for no good reason.
For example, on some dive computers, you might not *get* an index (because the dive computer fundamentally just reports O2 percentage in the event). So then it would be better to say "I don't know the index" than basically make up a possibly ambiguous cylinder.
Many dive computers store the gasmix index internally. That makes sense, because it requires fewer bytes (or even bits) compared to storing the o2/he percentages. The only exception to the rule are divesystem, suunto and shearwater. In that case we lookup the index of the gasmix.
If a dive computer lets you configure multiple identical gas mixes, and refers to them by their o2/he content instead of an index, then that's just stupid. There is no way to tell which mix was used, so why even bother to setup identical mixes? That makes no sense at all.
That's why for example the Shearwater doesn't even support this, and refuses to add a duplicate mix.
But you are referring to an ambiguous cylinder. That's a completely different thing. The DC_SAMPLE_GASMIX is about changes in the gasmix, and not the tank. If you have a dive computer that has no knowledge about tanks (e.g. a non air integrated model) and you use identical gas mixes as a substitute for tracking tank switches (e.g. sidemount), then you're out of luck if your dive computer doesn't store the index.
And I think some others (Petrel?) allow you to change the mix during the dive for a special cylinder (perhaps you set your mixes wrong, or perhaps you're breathing a buddy gas). Sure, you can give the index for the special cylinder, but what happens if the user changed the gasmix twice? I'm not sure what the dive computer reports, but from a abstraction standpoint it would be good to give both index and mix - at least there's no dropped information, even if it might also confuse some user of libdivecomputer.
I'm not sure whether the Petrel supports this, but the OSTC certainly does. There are 5 pre-configured mixes and a 6th mix which can be changed during the dive. In libdivecomputer I parse the profile and add an new mix whenever this 6th mix is changed. So it's no problem if it's changed multiple times, and those extra mixes have an index as well.
Jef
-
Dirk Hohndel -
Jef Driesen -
Linus Torvalds