tnt has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-e1-hardware/+/29609 )
Change subject: icE1usb fw/gpsdo: Use PID loop instead of ad-hoc algo
......................................................................
icE1usb fw/gpsdo: Use PID loop instead of ad-hoc algo
The original algo is just somehting I originally came up with
on the spot and worked and I never got the time to revisit.
Now after some testing, I implemented a PID loop that seems to
present faster lock time and at least as good stability.
Parameters were not thorougly optimized just some 'hand' tuning
trying to simulate the behavior.
Signed-off-by: Sylvain Munaut <tnt(a)246tNt.com>
Change-Id: Ie3ea7243aa4234585f1ace222632bb5580caca75
---
M firmware/ice40-riscv/icE1usb/gpsdo.c
1 file changed, 22 insertions(+), 28 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-e1-hardware refs/changes/09/29609/1
diff --git a/firmware/ice40-riscv/icE1usb/gpsdo.c b/firmware/ice40-riscv/icE1usb/gpsdo.c
index 5975100..573c0bb 100644
--- a/firmware/ice40-riscv/icE1usb/gpsdo.c
+++ b/firmware/ice40-riscv/icE1usb/gpsdo.c
@@ -49,8 +49,8 @@
/* Fine tracking */
struct {
- int acc;
- int div;
+ int acc; /* Accumulated error */
+ int integral; /* PID Integral term */
} fine;
} g_gpsdo;
@@ -60,6 +60,7 @@
* VCXTO parameters
*
* - iKv is reciprocal sensitivity vs 'coarse' count for fast initial acquisition
+ * - Kp, Ki, Kd are params for the fine-tracking PID loop
*
* Note that the spec if often a guaranteed minimum range and goes
* from ~0.1V to 3.2V instead of 0-3.3V so actual sensitivity is
@@ -67,13 +68,22 @@
*/
static const struct {
int iKv; /* hi-count / Hz (.8 fixed point) */
+ int Kp; /* PID proportional term (.12 fixed point) */
+ int Ki; /* PID integral term (.12 fixed point) */
+ int Kd; /* PID differential term (.12 fixed point) */
} vctxo_params[] = {
[VCTXO_TAITIEN_VT40] = {
.iKv = 300, /* +- 50 ppm pull range => ~ 0.75 Hz / hi-count (set to 0.85) */
+ .Kp = 14336,
+ .Ki = 410,
+ .Kd = -6144,
},
[VCTXO_SITIME_SIT3808_E] = {
.iKv = 160, /* +- 100 ppm pull range => ~ 1.50 Hz / hi-count (set to 1.6) */
+ .Kp = 7168,
+ .Ki = 205,
+ .Kd = -4096,
},
};
@@ -188,7 +198,7 @@
/* Reset the long term error tracking */
g_gpsdo.fine.acc = 0;
- g_gpsdo.fine.div = 0;
+ g_gpsdo.fine.integral = 0;
}
static void
@@ -219,7 +229,6 @@
_gpsdo_fine_track(uint32_t tick_diff)
{
int freq_diff = (int)tick_diff - TARGET;
- uint16_t tune;
/* Did we deviate too much ? */
if ((freq_diff < -2*MAX_DEV_FINE) || (freq_diff > 2*MAX_DEV_FINE)) {
@@ -227,30 +236,16 @@
return;
}
- /* Accumulate long term error */
+ /* Accumulate long term error and integrate it */
g_gpsdo.fine.acc += freq_diff;
+ g_gpsdo.fine.integral += g_gpsdo.fine.acc;
- /* Update fine tuning value */
- if (((g_gpsdo.fine.acc >= 0) && (freq_diff > 0)) ||
- ((g_gpsdo.fine.acc <= 0) && (freq_diff < 0)))
- {
- g_gpsdo.tune.fine -= freq_diff;
- }
-
- if (g_gpsdo.fine.acc) {
- if (++g_gpsdo.fine.div > 10) {
- g_gpsdo.fine.div = 0;
- if (g_gpsdo.fine.acc > 0)
- g_gpsdo.tune.fine--;
- else if (g_gpsdo.fine.acc < 0)
- g_gpsdo.tune.fine++;
- }
- } else {
- g_gpsdo.fine.div = 0;
- }
-
- /* Compute value with a bias from long term accumulator */
- tune = g_gpsdo.tune.fine - (g_gpsdo.fine.acc / 2);
+ /* PID */
+ g_gpsdo.tune.fine = 2048 - ((
+ vctxo_params[g_gpsdo.vctxo].Kp * g_gpsdo.fine.acc +
+ vctxo_params[g_gpsdo.vctxo].Ki * g_gpsdo.fine.integral +
+ vctxo_params[g_gpsdo.vctxo].Kd * freq_diff
+ ) >> 12);
/* If fine tune is getting close to boundary, do our
* best to transfer part of it to coarse tuning */
@@ -260,13 +255,12 @@
g_gpsdo.tune.coarse += coarse_adj;
g_gpsdo.tune.fine -= coarse_adj << 6;
- tune -= coarse_adj << 6;
pdm_set(PDM_CLK_HI, true, g_gpsdo.tune.coarse, false);
}
/* Apply fine */
- pdm_set(PDM_CLK_LO, true, tune, false);
+ pdm_set(PDM_CLK_LO, true, g_gpsdo.tune.fine, false);
/* Debug */
#ifdef GPSDO_DEBUG
--
To view, visit https://gerrit.osmocom.org/c/osmo-e1-hardware/+/29609
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-e1-hardware
Gerrit-Branch: master
Gerrit-Change-Id: Ie3ea7243aa4234585f1ace222632bb5580caca75
Gerrit-Change-Number: 29609
Gerrit-PatchSet: 1
Gerrit-Owner: tnt <tnt(a)246tNt.com>
Gerrit-MessageType: newchange
tnt has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-e1-hardware/+/29610 )
Change subject: icE1usb fw/gpsdo: If no PPS present for >= 3s, go to hold over mode
......................................................................
icE1usb fw/gpsdo: If no PPS present for >= 3s, go to hold over mode
Signed-off-by: Sylvain Munaut <tnt(a)246tNt.com>
Change-Id: Ia85a8bb0e146cb117ea6e2704c6a4dedf215c75a
---
M firmware/ice40-riscv/icE1usb/gpsdo.c
1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-e1-hardware refs/changes/10/29610/1
diff --git a/firmware/ice40-riscv/icE1usb/gpsdo.c b/firmware/ice40-riscv/icE1usb/gpsdo.c
index 573c0bb..2481c2e 100644
--- a/firmware/ice40-riscv/icE1usb/gpsdo.c
+++ b/firmware/ice40-riscv/icE1usb/gpsdo.c
@@ -15,6 +15,8 @@
#include "ice1usb_proto.h"
+#include "config.h"
+
struct {
/* Configuration */
@@ -275,6 +277,14 @@
uint32_t tick_now, tick_diff;
bool valid;
+ /* If more than 3 sec elapsed since last PPS, go to hold-over */
+ if (time_elapsed(g_gpsdo.meas.tick_prev, 3 * SYS_CLK_FREQ)) {
+ g_gpsdo.state = STATE_HOLD_OVER;
+ g_gpsdo.meas.invalid = 0;
+ g_gpsdo.meas.skip = 0;
+ return;
+ }
+
/* Get current tick and check if there was a PPS and estimate frequency */
tick_now = time_pps_read();
@@ -299,6 +309,7 @@
/* No GPS fix, go to hold-over */
g_gpsdo.state = STATE_HOLD_OVER;
g_gpsdo.meas.invalid = 0;
+ g_gpsdo.meas.skip = 0;
return;
}
--
To view, visit https://gerrit.osmocom.org/c/osmo-e1-hardware/+/29610
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-e1-hardware
Gerrit-Branch: master
Gerrit-Change-Id: Ia85a8bb0e146cb117ea6e2704c6a4dedf215c75a
Gerrit-Change-Number: 29610
Gerrit-PatchSet: 1
Gerrit-Owner: tnt <tnt(a)246tNt.com>
Gerrit-MessageType: newchange
Attention is currently required from: laforge, dexter.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/29591 )
Change subject: msgb: dont return a length when msgb->lXh is NULL
......................................................................
Patch Set 1: Code-Review-1
(1 comment)
Patchset:
PS1:
Let's better OSMO_ASSERT() there, I run myself into this sort of problem in the past and it's really costly to find out what's going on.
Maybe even better, use osmo_panic() explaining the error before asserting.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/29591
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I1795c559f190713ebbabfbabf3453ab77da46a49
Gerrit-Change-Number: 29591
Gerrit-PatchSet: 1
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 04 Oct 2022 14:05:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: pespin, fixeria.
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/29576 )
Change subject: osmux: Close osmux socket when bts is freed
......................................................................
Patch Set 4:
(1 comment)
File src/common/osmux.c:
https://gerrit.osmocom.org/c/osmo-bts/+/29576/comment/0367cbf6_64464cea
PS2, Line 301: close(bts->osmux.fd.fd);
> Ye,s Good catch!
why not use osmo_fd_close() instead of these three lines?
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/29576
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Ibd3faa33b28d45048c340b177f13d5685f41a784
Gerrit-Change-Number: 29576
Gerrit-PatchSet: 4
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 04 Oct 2022 14:03:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/29575 )
Change subject: osmux: Skip lchans in lookup which still have no remote associated
......................................................................
osmux: Skip lchans in lookup which still have no remote associated
Lchans which are marked as non-connected have not yet received
information about its remote peer, hence they may not have some fields
available yet. Let's skip them to avoid accessing such fields
(lchan->abis_ip.osmux.in).
Related: SYS#5987
Change-Id: Id53822c4a0486b0090df2db3d185e047d14fc90a
---
M src/common/osmux.c
1 file changed, 2 insertions(+), 0 deletions(-)
Approvals:
laforge: Looks good to me, but someone else must approve
fixeria: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/src/common/osmux.c b/src/common/osmux.c
index d10d091..498fe24 100644
--- a/src/common/osmux.c
+++ b/src/common/osmux.c
@@ -237,6 +237,8 @@
struct osmux_handle *h;
if (!lchan->abis_ip.osmux.use)
continue;
+ if (!lchan_osmux_connected(lchan))
+ continue;
if (lchan->abis_ip.osmux.local_cid != osmux_cid)
continue;
h = osmux_xfrm_input_get_deliver_cb_data(lchan->abis_ip.osmux.in);
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/29575
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Id53822c4a0486b0090df2db3d185e047d14fc90a
Gerrit-Change-Number: 29575
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged