Merge pull request #12903 from ctiller/pid++
C++ize PidController
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 8e65310..f5a50ac 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -543,7 +543,6 @@
add_dependencies(buildtests_c timer_list_test)
add_dependencies(buildtests_c transport_connectivity_state_test)
add_dependencies(buildtests_c transport_metadata_test)
-add_dependencies(buildtests_c transport_pid_controller_test)
if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX)
add_dependencies(buildtests_c transport_security_test)
endif()
@@ -758,6 +757,7 @@
add_dependencies(buildtests_cxx stress_test)
add_dependencies(buildtests_cxx thread_manager_test)
add_dependencies(buildtests_cxx thread_stress_test)
+add_dependencies(buildtests_cxx transport_pid_controller_test)
add_dependencies(buildtests_cxx vector_test)
if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX)
add_dependencies(buildtests_cxx writes_per_rpc_test)
@@ -9147,36 +9147,6 @@
endif (gRPC_BUILD_TESTS)
if (gRPC_BUILD_TESTS)
-
-add_executable(transport_pid_controller_test
- test/core/transport/pid_controller_test.c
-)
-
-
-target_include_directories(transport_pid_controller_test
- PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}
- PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include
- PRIVATE ${BORINGSSL_ROOT_DIR}/include
- PRIVATE ${PROTOBUF_ROOT_DIR}/src
- PRIVATE ${BENCHMARK_ROOT_DIR}/include
- PRIVATE ${ZLIB_ROOT_DIR}
- PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/zlib
- PRIVATE ${CARES_INCLUDE_DIR}
- PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/cares/cares
- PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/gflags/include
- PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/third_party/abseil-cpp
-)
-
-target_link_libraries(transport_pid_controller_test
- ${_gRPC_ALLTARGETS_LIBRARIES}
- grpc_test_util
- grpc
- gpr_test_util
- gpr
-)
-
-endif (gRPC_BUILD_TESTS)
-if (gRPC_BUILD_TESTS)
if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX)
add_executable(transport_security_test
@@ -12967,6 +12937,47 @@
endif (gRPC_BUILD_TESTS)
if (gRPC_BUILD_TESTS)
+add_executable(transport_pid_controller_test
+ test/core/transport/pid_controller_test.cc
+ third_party/googletest/googletest/src/gtest-all.cc
+ third_party/googletest/googlemock/src/gmock-all.cc
+)
+
+
+target_include_directories(transport_pid_controller_test
+ PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}
+ PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include
+ PRIVATE ${BORINGSSL_ROOT_DIR}/include
+ PRIVATE ${PROTOBUF_ROOT_DIR}/src
+ PRIVATE ${BENCHMARK_ROOT_DIR}/include
+ PRIVATE ${ZLIB_ROOT_DIR}
+ PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/zlib
+ PRIVATE ${CARES_INCLUDE_DIR}
+ PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/cares/cares
+ PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/gflags/include
+ PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/third_party/abseil-cpp
+ PRIVATE third_party/googletest/googletest/include
+ PRIVATE third_party/googletest/googletest
+ PRIVATE third_party/googletest/googlemock/include
+ PRIVATE third_party/googletest/googlemock
+ PRIVATE ${_gRPC_PROTO_GENS_DIR}
+)
+
+target_link_libraries(transport_pid_controller_test
+ ${_gRPC_PROTOBUF_LIBRARIES}
+ ${_gRPC_ALLTARGETS_LIBRARIES}
+ grpc++_test_util
+ grpc++
+ grpc_test_util
+ grpc
+ gpr_test_util
+ gpr
+ ${_gRPC_GFLAGS_LIBRARIES}
+)
+
+endif (gRPC_BUILD_TESTS)
+if (gRPC_BUILD_TESTS)
+
add_executable(vector_test
test/core/support/vector_test.cc
third_party/googletest/googletest/src/gtest-all.cc
diff --git a/Makefile b/Makefile
index cab0a95..047ce14 100644
--- a/Makefile
+++ b/Makefile
@@ -1091,7 +1091,6 @@
timer_list_test: $(BINDIR)/$(CONFIG)/timer_list_test
transport_connectivity_state_test: $(BINDIR)/$(CONFIG)/transport_connectivity_state_test
transport_metadata_test: $(BINDIR)/$(CONFIG)/transport_metadata_test
-transport_pid_controller_test: $(BINDIR)/$(CONFIG)/transport_pid_controller_test
transport_security_test: $(BINDIR)/$(CONFIG)/transport_security_test
udp_server_test: $(BINDIR)/$(CONFIG)/udp_server_test
uri_fuzzer_test: $(BINDIR)/$(CONFIG)/uri_fuzzer_test
@@ -1180,6 +1179,7 @@
stress_test: $(BINDIR)/$(CONFIG)/stress_test
thread_manager_test: $(BINDIR)/$(CONFIG)/thread_manager_test
thread_stress_test: $(BINDIR)/$(CONFIG)/thread_stress_test
+transport_pid_controller_test: $(BINDIR)/$(CONFIG)/transport_pid_controller_test
vector_test: $(BINDIR)/$(CONFIG)/vector_test
writes_per_rpc_test: $(BINDIR)/$(CONFIG)/writes_per_rpc_test
public_headers_must_be_c89: $(BINDIR)/$(CONFIG)/public_headers_must_be_c89
@@ -1473,7 +1473,6 @@
$(BINDIR)/$(CONFIG)/timer_list_test \
$(BINDIR)/$(CONFIG)/transport_connectivity_state_test \
$(BINDIR)/$(CONFIG)/transport_metadata_test \
- $(BINDIR)/$(CONFIG)/transport_pid_controller_test \
$(BINDIR)/$(CONFIG)/transport_security_test \
$(BINDIR)/$(CONFIG)/udp_server_test \
$(BINDIR)/$(CONFIG)/uri_parser_test \
@@ -1618,6 +1617,7 @@
$(BINDIR)/$(CONFIG)/stress_test \
$(BINDIR)/$(CONFIG)/thread_manager_test \
$(BINDIR)/$(CONFIG)/thread_stress_test \
+ $(BINDIR)/$(CONFIG)/transport_pid_controller_test \
$(BINDIR)/$(CONFIG)/vector_test \
$(BINDIR)/$(CONFIG)/writes_per_rpc_test \
$(BINDIR)/$(CONFIG)/boringssl_aes_test \
@@ -1741,6 +1741,7 @@
$(BINDIR)/$(CONFIG)/stress_test \
$(BINDIR)/$(CONFIG)/thread_manager_test \
$(BINDIR)/$(CONFIG)/thread_stress_test \
+ $(BINDIR)/$(CONFIG)/transport_pid_controller_test \
$(BINDIR)/$(CONFIG)/vector_test \
$(BINDIR)/$(CONFIG)/writes_per_rpc_test \
$(BINDIR)/$(CONFIG)/resolver_component_test_unsecure \
@@ -1994,8 +1995,6 @@
$(Q) $(BINDIR)/$(CONFIG)/transport_connectivity_state_test || ( echo test transport_connectivity_state_test failed ; exit 1 )
$(E) "[RUN] Testing transport_metadata_test"
$(Q) $(BINDIR)/$(CONFIG)/transport_metadata_test || ( echo test transport_metadata_test failed ; exit 1 )
- $(E) "[RUN] Testing transport_pid_controller_test"
- $(Q) $(BINDIR)/$(CONFIG)/transport_pid_controller_test || ( echo test transport_pid_controller_test failed ; exit 1 )
$(E) "[RUN] Testing transport_security_test"
$(Q) $(BINDIR)/$(CONFIG)/transport_security_test || ( echo test transport_security_test failed ; exit 1 )
$(E) "[RUN] Testing udp_server_test"
@@ -2158,6 +2157,8 @@
$(Q) $(BINDIR)/$(CONFIG)/thread_manager_test || ( echo test thread_manager_test failed ; exit 1 )
$(E) "[RUN] Testing thread_stress_test"
$(Q) $(BINDIR)/$(CONFIG)/thread_stress_test || ( echo test thread_stress_test failed ; exit 1 )
+ $(E) "[RUN] Testing transport_pid_controller_test"
+ $(Q) $(BINDIR)/$(CONFIG)/transport_pid_controller_test || ( echo test transport_pid_controller_test failed ; exit 1 )
$(E) "[RUN] Testing vector_test"
$(Q) $(BINDIR)/$(CONFIG)/vector_test || ( echo test vector_test failed ; exit 1 )
$(E) "[RUN] Testing writes_per_rpc_test"
@@ -13424,38 +13425,6 @@
endif
-TRANSPORT_PID_CONTROLLER_TEST_SRC = \
- test/core/transport/pid_controller_test.c \
-
-TRANSPORT_PID_CONTROLLER_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(TRANSPORT_PID_CONTROLLER_TEST_SRC))))
-ifeq ($(NO_SECURE),true)
-
-# You can't build secure targets if you don't have OpenSSL.
-
-$(BINDIR)/$(CONFIG)/transport_pid_controller_test: openssl_dep_error
-
-else
-
-
-
-$(BINDIR)/$(CONFIG)/transport_pid_controller_test: $(TRANSPORT_PID_CONTROLLER_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a
- $(E) "[LD] Linking $@"
- $(Q) mkdir -p `dirname $@`
- $(Q) $(LD) $(LDFLAGS) $(TRANSPORT_PID_CONTROLLER_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBS) $(LDLIBS_SECURE) -o $(BINDIR)/$(CONFIG)/transport_pid_controller_test
-
-endif
-
-$(OBJDIR)/$(CONFIG)/test/core/transport/pid_controller_test.o: $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a
-
-deps_transport_pid_controller_test: $(TRANSPORT_PID_CONTROLLER_TEST_OBJS:.o=.dep)
-
-ifneq ($(NO_SECURE),true)
-ifneq ($(NO_DEPS),true)
--include $(TRANSPORT_PID_CONTROLLER_TEST_OBJS:.o=.dep)
-endif
-endif
-
-
TRANSPORT_SECURITY_TEST_SRC = \
test/core/tsi/transport_security_test.c \
@@ -17206,6 +17175,49 @@
endif
+TRANSPORT_PID_CONTROLLER_TEST_SRC = \
+ test/core/transport/pid_controller_test.cc \
+
+TRANSPORT_PID_CONTROLLER_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(TRANSPORT_PID_CONTROLLER_TEST_SRC))))
+ifeq ($(NO_SECURE),true)
+
+# You can't build secure targets if you don't have OpenSSL.
+
+$(BINDIR)/$(CONFIG)/transport_pid_controller_test: openssl_dep_error
+
+else
+
+
+
+
+ifeq ($(NO_PROTOBUF),true)
+
+# You can't build the protoc plugins or protobuf-enabled targets if you don't have protobuf 3.0.0+.
+
+$(BINDIR)/$(CONFIG)/transport_pid_controller_test: protobuf_dep_error
+
+else
+
+$(BINDIR)/$(CONFIG)/transport_pid_controller_test: $(PROTOBUF_DEP) $(TRANSPORT_PID_CONTROLLER_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a
+ $(E) "[LD] Linking $@"
+ $(Q) mkdir -p `dirname $@`
+ $(Q) $(LDXX) $(LDFLAGS) $(TRANSPORT_PID_CONTROLLER_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/transport_pid_controller_test
+
+endif
+
+endif
+
+$(OBJDIR)/$(CONFIG)/test/core/transport/pid_controller_test.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a
+
+deps_transport_pid_controller_test: $(TRANSPORT_PID_CONTROLLER_TEST_OBJS:.o=.dep)
+
+ifneq ($(NO_SECURE),true)
+ifneq ($(NO_DEPS),true)
+-include $(TRANSPORT_PID_CONTROLLER_TEST_OBJS:.o=.dep)
+endif
+endif
+
+
VECTOR_TEST_SRC = \
test/core/support/vector_test.cc \
diff --git a/build.yaml b/build.yaml
index 557025d..af741c0 100644
--- a/build.yaml
+++ b/build.yaml
@@ -3406,18 +3406,6 @@
- grpc
- gpr_test_util
- gpr
- uses_polling: false
-- name: transport_pid_controller_test
- build: test
- language: c
- src:
- - test/core/transport/pid_controller_test.c
- deps:
- - grpc_test_util
- - grpc
- - gpr_test_util
- - gpr
- uses_polling: false
- name: transport_security_test
build: test
language: c
@@ -4819,6 +4807,18 @@
- gpr_test_util
- gpr
timeout_seconds: 1200
+- name: transport_pid_controller_test
+ build: test
+ language: c++
+ src:
+ - test/core/transport/pid_controller_test.cc
+ deps:
+ - grpc++_test_util
+ - grpc++
+ - grpc_test_util
+ - grpc
+ - gpr_test_util
+ - gpr
- name: vector_test
gtest: true
build: test
diff --git a/src/core/ext/transport/chttp2/transport/flow_control.cc b/src/core/ext/transport/chttp2/transport/flow_control.cc
index d0e80c4..716cd71 100644
--- a/src/core/ext/transport/chttp2/transport/flow_control.cc
+++ b/src/core/ext/transport/chttp2/transport/flow_control.cc
@@ -399,22 +399,19 @@
if (!tfc->pid_controller_initialized) {
tfc->last_pid_update = now;
tfc->pid_controller_initialized = true;
- grpc_pid_controller_args args;
- memset(&args, 0, sizeof(args));
- args.gain_p = 4;
- args.gain_i = 8;
- args.gain_d = 0;
- args.initial_control_value = target;
- args.min_control_value = -1;
- args.max_control_value = 25;
- args.integral_range = 10;
- grpc_pid_controller_init(&tfc->pid_controller, args);
+ tfc->pid_controller.Init(grpc_core::PidController::Args()
+ .set_gain_p(4)
+ .set_gain_i(8)
+ .set_gain_d(0)
+ .set_initial_control_value(target)
+ .set_min_control_value(-1)
+ .set_max_control_value(25)
+ .set_integral_range(10));
return pow(2, target);
}
- double bdp_error = target - grpc_pid_controller_last(&tfc->pid_controller);
+ double bdp_error = target - tfc->pid_controller->last_control_value();
double dt = (double)(now - tfc->last_pid_update) * 1e-3;
- double log2_bdp_guess =
- grpc_pid_controller_update(&tfc->pid_controller, bdp_error, dt);
+ double log2_bdp_guess = tfc->pid_controller->Update(bdp_error, dt);
tfc->last_pid_update = now;
return pow(2, log2_bdp_guess);
}
diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h
index 703f3ba..c75f813 100644
--- a/src/core/ext/transport/chttp2/transport/internal.h
+++ b/src/core/ext/transport/chttp2/transport/internal.h
@@ -273,7 +273,7 @@
/* pid controller */
bool pid_controller_initialized;
- grpc_pid_controller pid_controller;
+ grpc_core::ManualConstructor<grpc_core::PidController> pid_controller;
grpc_millis last_pid_update;
// pointer back to transport for tracing
diff --git a/src/core/lib/transport/pid_controller.cc b/src/core/lib/transport/pid_controller.cc
index 4b304f1..9f7750d 100644
--- a/src/core/lib/transport/pid_controller.cc
+++ b/src/core/lib/transport/pid_controller.cc
@@ -19,45 +19,30 @@
#include "src/core/lib/transport/pid_controller.h"
#include <grpc/support/useful.h>
-void grpc_pid_controller_init(grpc_pid_controller *pid_controller,
- grpc_pid_controller_args args) {
- pid_controller->args = args;
- pid_controller->last_control_value = args.initial_control_value;
- grpc_pid_controller_reset(pid_controller);
-}
+namespace grpc_core {
-void grpc_pid_controller_reset(grpc_pid_controller *pid_controller) {
- pid_controller->last_error = 0.0;
- pid_controller->last_dc_dt = 0.0;
- pid_controller->error_integral = 0.0;
-}
+PidController::PidController(const Args &args)
+ : last_control_value_(args.initial_control_value()), args_(args) {}
-double grpc_pid_controller_update(grpc_pid_controller *pid_controller,
- double error, double dt) {
- if (dt == 0) return pid_controller->last_control_value;
+double PidController::Update(double error, double dt) {
+ if (dt <= 0) return last_control_value_;
/* integrate error using the trapezoid rule */
- pid_controller->error_integral +=
- dt * (pid_controller->last_error + error) * 0.5;
- pid_controller->error_integral = GPR_CLAMP(
- pid_controller->error_integral, -pid_controller->args.integral_range,
- pid_controller->args.integral_range);
- double diff_error = (error - pid_controller->last_error) / dt;
+ error_integral_ += dt * (last_error_ + error) * 0.5;
+ error_integral_ = GPR_CLAMP(error_integral_, -args_.integral_range(),
+ args_.integral_range());
+ double diff_error = (error - last_error_) / dt;
/* calculate derivative of control value vs time */
- double dc_dt = pid_controller->args.gain_p * error +
- pid_controller->args.gain_i * pid_controller->error_integral +
- pid_controller->args.gain_d * diff_error;
+ double dc_dt = args_.gain_p() * error + args_.gain_i() * error_integral_ +
+ args_.gain_d() * diff_error;
/* and perform trapezoidal integration */
- double new_control_value = pid_controller->last_control_value +
- dt * (pid_controller->last_dc_dt + dc_dt) * 0.5;
- new_control_value =
- GPR_CLAMP(new_control_value, pid_controller->args.min_control_value,
- pid_controller->args.max_control_value);
- pid_controller->last_error = error;
- pid_controller->last_dc_dt = dc_dt;
- pid_controller->last_control_value = new_control_value;
+ double new_control_value =
+ last_control_value_ + dt * (last_dc_dt_ + dc_dt) * 0.5;
+ new_control_value = GPR_CLAMP(new_control_value, args_.min_control_value(),
+ args_.max_control_value());
+ last_error_ = error;
+ last_dc_dt_ = dc_dt;
+ last_control_value_ = new_control_value;
return new_control_value;
}
-double grpc_pid_controller_last(grpc_pid_controller *pid_controller) {
- return pid_controller->last_control_value;
-}
+} // namespace grpc_core
diff --git a/src/core/lib/transport/pid_controller.h b/src/core/lib/transport/pid_controller.h
index 80899e9..87e59a1 100644
--- a/src/core/lib/transport/pid_controller.h
+++ b/src/core/lib/transport/pid_controller.h
@@ -19,9 +19,7 @@
#ifndef GRPC_CORE_LIB_TRANSPORT_PID_CONTROLLER_H
#define GRPC_CORE_LIB_TRANSPORT_PID_CONTROLLER_H
-#ifdef __cplusplus
-extern "C" {
-#endif
+#include <limits>
/* \file Simple PID controller.
Implements a proportional-integral-derivative controller.
@@ -30,41 +28,87 @@
Gains can be set to adjust sensitivity to current error (p), the integral
of error (i), and the derivative of error (d). */
-typedef struct {
- double gain_p;
- double gain_i;
- double gain_d;
- double initial_control_value;
- double min_control_value;
- double max_control_value;
- double integral_range;
-} grpc_pid_controller_args;
+namespace grpc_core {
-typedef struct {
- double last_error;
- double error_integral;
- double last_control_value;
- double last_dc_dt;
- grpc_pid_controller_args args;
-} grpc_pid_controller;
+class PidController {
+ public:
+ class Args {
+ public:
+ double gain_p() const { return gain_p_; }
+ double gain_i() const { return gain_i_; }
+ double gain_d() const { return gain_d_; }
+ double initial_control_value() const { return initial_control_value_; }
+ double min_control_value() const { return min_control_value_; }
+ double max_control_value() const { return max_control_value_; }
+ double integral_range() const { return integral_range_; }
-/** Initialize the controller */
-void grpc_pid_controller_init(grpc_pid_controller *pid_controller,
- grpc_pid_controller_args args);
+ Args& set_gain_p(double gain_p) {
+ gain_p_ = gain_p;
+ return *this;
+ }
+ Args& set_gain_i(double gain_i) {
+ gain_i_ = gain_i;
+ return *this;
+ }
+ Args& set_gain_d(double gain_d) {
+ gain_d_ = gain_d;
+ return *this;
+ }
+ Args& set_initial_control_value(double initial_control_value) {
+ initial_control_value_ = initial_control_value;
+ return *this;
+ }
+ Args& set_min_control_value(double min_control_value) {
+ min_control_value_ = min_control_value;
+ return *this;
+ }
+ Args& set_max_control_value(double max_control_value) {
+ max_control_value_ = max_control_value;
+ return *this;
+ }
+ Args& set_integral_range(double integral_range) {
+ integral_range_ = integral_range;
+ return *this;
+ }
-/** Reset the controller: useful when things have changed significantly */
-void grpc_pid_controller_reset(grpc_pid_controller *pid_controller);
+ private:
+ double gain_p_ = 0.0;
+ double gain_i_ = 0.0;
+ double gain_d_ = 0.0;
+ double initial_control_value_ = 0.0;
+ double min_control_value_ = std::numeric_limits<double>::min();
+ double max_control_value_ = std::numeric_limits<double>::max();
+ double integral_range_ = std::numeric_limits<double>::max();
+ };
-/** Update the controller: given a current error estimate, and the time since
- the last update, returns a new control value */
-double grpc_pid_controller_update(grpc_pid_controller *pid_controller,
- double error, double dt);
+ explicit PidController(const Args& args);
-/** Returns the last control value calculated */
-double grpc_pid_controller_last(grpc_pid_controller *pid_controller);
+ /// Reset the controller internal state: useful when the environment has
+ /// changed significantly
+ void Reset() {
+ last_error_ = 0.0;
+ last_dc_dt_ = 0.0;
+ error_integral_ = 0.0;
+ }
-#ifdef __cplusplus
-}
-#endif
+ /// Update the controller: given a current error estimate, and the time since
+ /// the last update, returns a new control value
+ double Update(double error, double dt);
+
+ /// Returns the last control value calculated
+ double last_control_value() const { return last_control_value_; }
+
+ /// Returns the current error integral (mostly for testing)
+ double error_integral() const { return error_integral_; }
+
+ private:
+ double last_error_ = 0.0;
+ double error_integral_ = 0.0;
+ double last_control_value_;
+ double last_dc_dt_ = 0.0;
+ const Args args_;
+};
+
+} // namespace grpc_core
#endif /* GRPC_CORE_LIB_TRANSPORT_PID_CONTROLLER_H */
diff --git a/test/core/transport/BUILD b/test/core/transport/BUILD
index ea5e577..141381b 100644
--- a/test/core/transport/BUILD
+++ b/test/core/transport/BUILD
@@ -71,7 +71,7 @@
grpc_cc_test(
name = "pid_controller_test",
- srcs = ["pid_controller_test.c"],
+ srcs = ["pid_controller_test.cc"],
language = "C",
deps = [
"//:gpr",
@@ -79,6 +79,9 @@
"//test/core/util:gpr_test_util",
"//test/core/util:grpc_test_util",
],
+ external_deps = [
+ "gtest",
+ ],
)
grpc_cc_test(
diff --git a/test/core/transport/bdp_estimator_test.cc b/test/core/transport/bdp_estimator_test.cc
index 7ac3501..a944762 100644
--- a/test/core/transport/bdp_estimator_test.cc
+++ b/test/core/transport/bdp_estimator_test.cc
@@ -143,6 +143,7 @@
INSTANTIATE_TEST_CASE_P(TooManyNames, BdpEstimatorRandomTest,
::testing::Values(3, 4, 6, 9, 13, 19, 28, 42, 63, 94,
141, 211, 316, 474, 711));
+
} // namespace testing
} // namespace grpc_core
diff --git a/test/core/transport/pid_controller_test.c b/test/core/transport/pid_controller_test.c
deleted file mode 100644
index 831c4b4..0000000
--- a/test/core/transport/pid_controller_test.c
+++ /dev/null
@@ -1,78 +0,0 @@
-/*
- *
- * Copyright 2016 gRPC authors.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- *
- */
-
-#include "src/core/lib/transport/pid_controller.h"
-
-#include <float.h>
-#include <math.h>
-
-#include <grpc/support/alloc.h>
-#include <grpc/support/log.h>
-#include <grpc/support/string_util.h>
-#include <grpc/support/useful.h>
-#include "src/core/lib/support/string.h"
-#include "test/core/util/test_config.h"
-
-static void test_noop(void) {
- gpr_log(GPR_INFO, "test_noop");
- grpc_pid_controller pid;
- grpc_pid_controller_init(
- &pid, (grpc_pid_controller_args){.gain_p = 1,
- .gain_i = 1,
- .gain_d = 1,
- .initial_control_value = 1,
- .min_control_value = DBL_MIN,
- .max_control_value = DBL_MAX,
- .integral_range = DBL_MAX});
-}
-
-static void test_simple_convergence(double gain_p, double gain_i, double gain_d,
- double dt, double set_point, double start) {
- gpr_log(GPR_INFO,
- "test_simple_convergence(p=%lf, i=%lf, d=%lf); dt=%lf set_point=%lf "
- "start=%lf",
- gain_p, gain_i, gain_d, dt, set_point, start);
- grpc_pid_controller pid;
- grpc_pid_controller_init(
- &pid, (grpc_pid_controller_args){.gain_p = gain_p,
- .gain_i = gain_i,
- .gain_d = gain_d,
- .initial_control_value = start,
- .min_control_value = DBL_MIN,
- .max_control_value = DBL_MAX,
- .integral_range = DBL_MAX});
-
- for (int i = 0; i < 100000; i++) {
- grpc_pid_controller_update(&pid, set_point - grpc_pid_controller_last(&pid),
- 1);
- }
-
- GPR_ASSERT(fabs(set_point - grpc_pid_controller_last(&pid)) < 0.1);
- if (gain_i > 0) {
- GPR_ASSERT(fabs(pid.error_integral) < 0.1);
- }
-}
-
-int main(int argc, char **argv) {
- grpc_test_init(argc, argv);
- test_noop();
- test_simple_convergence(0.2, 0, 0, 1, 100, 0);
- test_simple_convergence(0.2, 0.1, 0, 1, 100, 0);
- test_simple_convergence(0.2, 0.1, 0.1, 1, 100, 0);
- return 0;
-}
diff --git a/test/core/transport/pid_controller_test.cc b/test/core/transport/pid_controller_test.cc
new file mode 100644
index 0000000..081d034
--- /dev/null
+++ b/test/core/transport/pid_controller_test.cc
@@ -0,0 +1,91 @@
+/*
+ *
+ * Copyright 2016 gRPC authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+#include "src/core/lib/transport/pid_controller.h"
+
+#include <float.h>
+#include <math.h>
+
+#include <grpc/support/alloc.h>
+#include <grpc/support/log.h>
+#include <grpc/support/string_util.h>
+#include <grpc/support/useful.h>
+#include <gtest/gtest.h>
+#include "src/core/lib/support/string.h"
+#include "test/core/util/test_config.h"
+
+namespace grpc_core {
+namespace testing {
+
+TEST(PidController, NoOp) {
+ PidController pid(PidController::Args()
+ .set_gain_p(1)
+ .set_gain_i(1)
+ .set_gain_d(1)
+ .set_initial_control_value(1));
+}
+
+struct SimpleConvergenceTestArgs {
+ double gain_p;
+ double gain_i;
+ double gain_d;
+ double dt;
+ double set_point;
+ double start;
+};
+
+std::ostream& operator<<(std::ostream& out, SimpleConvergenceTestArgs args) {
+ return out << "gain_p:" << args.gain_p << " gain_i:" << args.gain_i
+ << " gain_d:" << args.gain_d << " dt:" << args.dt
+ << " set_point:" << args.set_point << " start:" << args.start;
+}
+
+class SimpleConvergenceTest
+ : public ::testing::TestWithParam<SimpleConvergenceTestArgs> {};
+
+TEST_P(SimpleConvergenceTest, Converges) {
+ PidController pid(PidController::Args()
+ .set_gain_p(GetParam().gain_p)
+ .set_gain_i(GetParam().gain_i)
+ .set_gain_d(GetParam().gain_d)
+ .set_initial_control_value(GetParam().start));
+
+ for (int i = 0; i < 100000; i++) {
+ pid.Update(GetParam().set_point - pid.last_control_value(), GetParam().dt);
+ }
+
+ EXPECT_LT(fabs(GetParam().set_point - pid.last_control_value()), 0.1);
+ if (GetParam().gain_i > 0) {
+ EXPECT_LT(fabs(pid.error_integral()), 0.1);
+ }
+}
+
+INSTANTIATE_TEST_CASE_P(
+ X, SimpleConvergenceTest,
+ ::testing::Values(SimpleConvergenceTestArgs{0.2, 0, 0, 1, 100, 0},
+ SimpleConvergenceTestArgs{0.2, 0.1, 0, 1, 100, 0},
+ SimpleConvergenceTestArgs{0.2, 0.1, 0.1, 1, 100, 0}));
+
+} // namespace testing
+} // namespace grpc_core
+
+int main(int argc, char** argv) {
+ grpc_test_init(argc, argv);
+ ::testing::InitGoogleTest(&argc, argv);
+ return RUN_ALL_TESTS();
+}
diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json
index dca2e83..e47b27e 100644
--- a/tools/run_tests/generated/sources_and_headers.json
+++ b/tools/run_tests/generated/sources_and_headers.json
@@ -2447,23 +2447,6 @@
"headers": [],
"is_filegroup": false,
"language": "c",
- "name": "transport_pid_controller_test",
- "src": [
- "test/core/transport/pid_controller_test.c"
- ],
- "third_party": false,
- "type": "target"
- },
- {
- "deps": [
- "gpr",
- "gpr_test_util",
- "grpc",
- "grpc_test_util"
- ],
- "headers": [],
- "is_filegroup": false,
- "language": "c",
"name": "transport_security_test",
"src": [
"test/core/tsi/transport_security_test.c"
@@ -4248,6 +4231,25 @@
"gpr_test_util",
"grpc",
"grpc++",
+ "grpc++_test_util",
+ "grpc_test_util"
+ ],
+ "headers": [],
+ "is_filegroup": false,
+ "language": "c++",
+ "name": "transport_pid_controller_test",
+ "src": [
+ "test/core/transport/pid_controller_test.cc"
+ ],
+ "third_party": false,
+ "type": "target"
+ },
+ {
+ "deps": [
+ "gpr",
+ "gpr_test_util",
+ "grpc",
+ "grpc++",
"grpc++_test",
"grpc_test_util"
],
diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json
index d819170..f65cdd8 100644
--- a/tools/run_tests/generated/tests.json
+++ b/tools/run_tests/generated/tests.json
@@ -2867,31 +2867,7 @@
"posix",
"windows"
],
- "uses_polling": false
- },
- {
- "args": [],
- "benchmark": false,
- "ci_platforms": [
- "linux",
- "mac",
- "posix",
- "windows"
- ],
- "cpu_cost": 1.0,
- "exclude_configs": [],
- "exclude_iomgrs": [],
- "flaky": false,
- "gtest": false,
- "language": "c",
- "name": "transport_pid_controller_test",
- "platforms": [
- "linux",
- "mac",
- "posix",
- "windows"
- ],
- "uses_polling": false
+ "uses_polling": true
},
{
"args": [],
@@ -4514,6 +4490,30 @@
"exclude_configs": [],
"exclude_iomgrs": [],
"flaky": false,
+ "gtest": false,
+ "language": "c++",
+ "name": "transport_pid_controller_test",
+ "platforms": [
+ "linux",
+ "mac",
+ "posix",
+ "windows"
+ ],
+ "uses_polling": true
+ },
+ {
+ "args": [],
+ "benchmark": false,
+ "ci_platforms": [
+ "linux",
+ "mac",
+ "posix",
+ "windows"
+ ],
+ "cpu_cost": 1.0,
+ "exclude_configs": [],
+ "exclude_iomgrs": [],
+ "flaky": false,
"gtest": true,
"language": "c++",
"name": "vector_test",