Skip to content

Commit

Permalink
Merge pull request grpc#12903 from ctiller/pid++
Browse files Browse the repository at this point in the history 
C++ize PidController
  • Loading branch information
@ctiller
ctiller authored Oct 18, 2017
2 parents 1167655 + ff84ae8 commit 0d11508
Show file tree
Hide file tree
Showing 13 changed files with 349 additions and 281 deletions.
73 changes: 42 additions & 31 deletions CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,6 @@ add_dependencies(buildtests_c timer_heap_test)
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()
Expand Down Expand Up @@ -758,6 +757,7 @@ endif()
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)
Expand Down Expand Up @@ -9145,36 +9145,6 @@ target_link_libraries(transport_metadata_test
gpr
)

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)
Expand Down Expand Up @@ -12967,6 +12937,47 @@ target_link_libraries(thread_stress_test
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
Expand Down
84 changes: 48 additions & 36 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1091,7 +1091,6 @@ timer_heap_test: $(BINDIR)/$(CONFIG)/timer_heap_test
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
Expand Down Expand Up @@ -1180,6 +1179,7 @@ streaming_throughput_test: $(BINDIR)/$(CONFIG)/streaming_throughput_test
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
Expand Down Expand Up @@ -1473,7 +1473,6 @@ buildtests_c: privatelibs_c \
$(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 \
Expand Down Expand Up @@ -1618,6 +1617,7 @@ buildtests_cxx: privatelibs_cxx \
$(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 \
Expand Down Expand Up @@ -1741,6 +1741,7 @@ buildtests_cxx: privatelibs_cxx \
$(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 \
Expand Down Expand Up @@ -1994,8 +1995,6 @@ test_c: buildtests_c
$(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"
Expand Down Expand Up @@ -2158,6 +2157,8 @@ test_cxx: buildtests_cxx
$(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"
Expand Down Expand Up @@ -13424,38 +13425,6 @@ endif
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 \

Expand Down Expand Up @@ -17206,6 +17175,49 @@ endif
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 \

Expand Down
24 changes: 12 additions & 12 deletions build.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3406,18 +3406,6 @@ targets:
- 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
Expand Down Expand Up @@ -4819,6 +4807,18 @@ targets:
- 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
Expand Down
23 changes: 10 additions & 13 deletions src/core/ext/transport/chttp2/transport/flow_control.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -399,22 +399,19 @@ static double get_pid_controller_guess(grpc_exec_ctx* exec_ctx,
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);
}
Expand Down
2 changes: 1 addition & 1 deletion src/core/ext/transport/chttp2/transport/internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ typedef struct {

/* 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
Expand Down
53 changes: 19 additions & 34 deletions src/core/lib/transport/pid_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading

0 comments on commit 0d11508

Please sign in to comment.