Skip to content
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

v1.1.0 #20

Merged
merged 16 commits into from
Dec 19, 2022
Merged

v1.1.0 #20

merged 16 commits into from
Dec 19, 2022

Conversation

cyberman54
Copy link
Contributor

fix issue #19

@cyberman54
Copy link
Contributor Author

@oliverbrandmueller ping

@@ -31,7 +31,7 @@ struct libpax_config_t {
// <- 13 ..........1 ->
// 0b1010000001001 would be Channel: 1, 4, 11, 13
uint8_t wificounter; // set to 0 if you do not want to install the WiFi sniffer
uint8_t wifi_my_country; // e.g 0 = "EU", etc. select locale for WiFi RF settings
char wifi_my_country[3]; // set country code for WiFi RF settings, e.g. "01", "DE", etc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break libpax_serialize_config / libpax_deserialize_config for reading existing configs as all fields after wifi_my_country are moved by two bytes.

I see two options:

  1. release as breaking change
  • to follow semantic versioning this should be 2.0.0 and not 1.0.2 to signal that it is a breaking change (to API as well as stored configs)
  • increase CONFIG_MAJOR_VERSION
  1. release with backwards-compatibility
  • add a wifi_my_country_str (or similar) at the end of the struct and keep the uint8_t wifi_my_country
  • if wifi_my_country_str is not set read wifi_my_country and convert to wifi_my_country_str
  • reduce libpax_config_storage_t.reserved_end from 25 bytes to 22 to keep the size of libpax_config_storage_t constant
  • increase CONFIG_MINOR_VERSION

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I go for no 2. and added commit 0f6c9dc to keep backward compatibility.
@FlorianLudwig Please check, thanks!

@cyberman54 cyberman54 changed the title v1.0.2 v1.1.0 Oct 29, 2022
@cyberman54
Copy link
Contributor Author

Added a fix to solve issue #21

@lifezoned4
Copy link
Contributor

the code LGTM but one of the unit test fails because the counting of the report intervals is not summing up to 6 in 6 sec (1 sec per report) for espidf env. I think this has to do with the espressif32@5.2.0 version bump. For now I would say we give the delay 10ms more in test/libpax_test_cases.cpp:125. I don't see any problem in the actually timing when looking at esp_timer_get_time values in between the reports. So I assume it is a problem with vTaskDelayUntil?

The arduino env works fine without any unit test change.

Perhaps Also bump the version in library.json to 1.1.0 so we can go for a release?

@cyberman54
Copy link
Contributor Author

@lifezoned4 fixed it, thanks for pointing to the Vtaskdelay timer. I set it to 6010 now. Will try to find out what's the root cause here.

@cyberman54
Copy link
Contributor Author

This PR should now be ready to merge.
/cc @lifezoned4

@oliverbrandmueller oliverbrandmueller merged commit fe98131 into dbinfrago:master Dec 19, 2022
@cyberman54
Copy link
Contributor Author

@oliverbrandmueller for this PR we need the Release 1.1.0, too.

@cyberman54
Copy link
Contributor Author

@oliverbrandmueller Platformio library manager does not find v1.1.0, does it need a kick?

image

cyberman54 added a commit to cyberman54/libpax that referenced this pull request Dec 21, 2022
Merge pull request dbinfrago#20 from cyberman54/master
@cyberman54
Copy link
Contributor Author

@FlorianLudwig @oliverbrandmueller We need a pio pkg publish for v1.1.0 of this lib by owner greyrook!

image

@lifezoned4
Copy link
Contributor

Did a publish bump and it should be available now. @cyberman54

@cyberman54
Copy link
Contributor Author

@lifezoned4 now it works in platformio, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants